Message ID | 20201001225933.1373426-1-kuba@kernel.org |
---|---|
Headers | show |
Series | genetlink: support per-command policy dump | expand |
On Thu, 1 Oct 2020 15:59:23 -0700 Jakub Kicinski wrote: > Hi! > > The objective of this series is to dump ethtool policies > to be able to tell which flags are supported by the kernel. > Current release adds ETHTOOL_FLAG_STATS for dumping extra > stats, but because of strict checking we need to make sure > that the flag is actually supported before setting it in > a request. Do we need support for separate .doit and .dumpit policies? Or is that an overkill?
On Thu, 2020-10-01 at 17:36 -0700, Jakub Kicinski wrote: > On Thu, 1 Oct 2020 15:59:23 -0700 Jakub Kicinski wrote: > > Hi! > > > > The objective of this series is to dump ethtool policies > > to be able to tell which flags are supported by the kernel. > > Current release adds ETHTOOL_FLAG_STATS for dumping extra > > stats, but because of strict checking we need to make sure > > that the flag is actually supported before setting it in > > a request. > > Do we need support for separate .doit and .dumpit policies? > Or is that an overkill? I suppose you could make an argument that only some attrs might be accepted in doit and somewhat others in dumpit, or perhaps none in dumpit because filtering wasn't implemented? But still ... often we treat filtering as "advisory" anyway (except perhaps where there's no doit at all, like the dump_policy thing here), so it wouldn't matter if some attribute is ending up ignored? johannes
On Fri, 02 Oct 2020 08:29:27 +0200 Johannes Berg wrote: > On Thu, 2020-10-01 at 17:36 -0700, Jakub Kicinski wrote: > > Do we need support for separate .doit and .dumpit policies? > > Or is that an overkill? > > I suppose you could make an argument that only some attrs might be > accepted in doit and somewhat others in dumpit, or perhaps none in > dumpit because filtering wasn't implemented? Right? Feels like it goes against our strict validation policy to ignore input on dumpit. > But still ... often we treat filtering as "advisory" anyway (except > perhaps where there's no doit at all, like the dump_policy thing here), > so it wouldn't matter if some attribute is ending up ignored? It may be useful for feature discovery to know if an attribute is supported. I don't think it matters for any user right now, but maybe we should require user space to specify if they are interested in normal req policy or dump policy? That'd give us the ability to report different ones in the future when the need arises.
On Fri, 2020-10-02 at 07:40 -0700, Jakub Kicinski wrote: > > I suppose you could make an argument that only some attrs might be > > accepted in doit and somewhat others in dumpit, or perhaps none in > > dumpit because filtering wasn't implemented? > > Right? Feels like it goes against our strict validation policy to > ignore input on dumpit. > > > But still ... often we treat filtering as "advisory" anyway (except > > perhaps where there's no doit at all, like the dump_policy thing here), > > so it wouldn't matter if some attribute is ending up ignored? > > It may be useful for feature discovery to know if an attribute is > supported. Fair point. > I don't think it matters for any user right now, but maybe we should > require user space to specify if they are interested in normal req > policy or dump policy? That'd give us the ability to report different > ones in the future when the need arises. Or just give them both? I mean, in many (most?) cases they're anyway going to be the same, so with the patches I posted you could just give them the two different policy indexes, and they can be the same? But whichever, doesn't really matter much. johannes
On Fri, 02 Oct 2020 16:42:09 +0200 Johannes Berg wrote: > On Fri, 2020-10-02 at 07:40 -0700, Jakub Kicinski wrote: > > > > I suppose you could make an argument that only some attrs might be > > > accepted in doit and somewhat others in dumpit, or perhaps none in > > > dumpit because filtering wasn't implemented? > > > > Right? Feels like it goes against our strict validation policy to > > ignore input on dumpit. > > > > > But still ... often we treat filtering as "advisory" anyway (except > > > perhaps where there's no doit at all, like the dump_policy thing here), > > > so it wouldn't matter if some attribute is ending up ignored? > > > > It may be useful for feature discovery to know if an attribute is > > supported. > > Fair point. > > > I don't think it matters for any user right now, but maybe we should > > require user space to specify if they are interested in normal req > > policy or dump policy? That'd give us the ability to report different > > ones in the future when the need arises. > > Or just give them both? I mean, in many (most?) cases they're anyway > going to be the same, so with the patches I posted you could just give > them the two different policy indexes, and they can be the same? Ah, I missed your posting! Like this? [OP_POLICY] [OP] [DO] -> u32 [DUMP] -> u32 > But whichever, doesn't really matter much.
> > Or just give them both? I mean, in many (most?) cases they're anyway > > going to be the same, so with the patches I posted you could just give > > them the two different policy indexes, and they can be the same? > > Ah, I missed your posting! Huh, I even CC'ed you I think? https://lore.kernel.org/netdev/20201002090944.195891-1-johannes@sipsolutions.net/t/#u and userspace: https://lore.kernel.org/netdev/20201002102609.224150-1-johannes@sipsolutions.net/t/#u > Like this? > > [OP_POLICY] > [OP] > [DO] -> u32 > [DUMP] -> u32 Yeah, that'd work. I'd probably wonder if we shouldn't do [OP_POLICY] [OP] -> (u32, u32) in a struct with two u32's, since that's quite a bit more compact. I did only: [OP_POLICY] [OP] -> u32 johannes
On Fri, 02 Oct 2020 16:58:33 +0200 Johannes Berg wrote: > > > Or just give them both? I mean, in many (most?) cases they're anyway > > > going to be the same, so with the patches I posted you could just give > > > them the two different policy indexes, and they can be the same? > > > > Ah, I missed your posting! > > Huh, I even CC'ed you I think? I filter stuff which is to:netdev cc:me and get to it when I read the ML. There's too much of it. > https://lore.kernel.org/netdev/20201002090944.195891-1-johannes@sipsolutions.net/t/#u > > and userspace: > > https://lore.kernel.org/netdev/20201002102609.224150-1-johannes@sipsolutions.net/t/#u > > > Like this? > > > > [OP_POLICY] > > [OP] > > [DO] -> u32 > > [DUMP] -> u32 > > Yeah, that'd work. I'd probably wonder if we shouldn't do > > [OP_POLICY] > [OP] -> (u32, u32) > > in a struct with two u32's, since that's quite a bit more compact. What do we do if the op doesn't have a dump or do callback? 0 is a valid policy ID, sadly :(
On Fri, 2020-10-02 at 08:03 -0700, Jakub Kicinski wrote: > > Huh, I even CC'ed you I think? > > I filter stuff which is to:netdev cc:me and get to it when I read the > ML. There's too much of it. Ah, ok :) > > Yeah, that'd work. I'd probably wonder if we shouldn't do > > > > [OP_POLICY] > > [OP] -> (u32, u32) > > > > in a struct with two u32's, since that's quite a bit more compact. > > What do we do if the op doesn't have a dump or do callback? > 0 is a valid policy ID, sadly :( Hm, good point. We could do -1 since that can't ever be reached though. But compactness isn't really that necessary here anyway, so ... johannes
On Fri, 02 Oct 2020 17:04:11 +0200 Johannes Berg wrote: > > > Yeah, that'd work. I'd probably wonder if we shouldn't do > > > > > > [OP_POLICY] > > > [OP] -> (u32, u32) > > > > > > in a struct with two u32's, since that's quite a bit more compact. > > > > What do we do if the op doesn't have a dump or do callback? > > 0 is a valid policy ID, sadly :( > > Hm, good point. We could do -1 since that can't ever be reached though. > > But compactness isn't really that necessary here anyway, so ... Cool, sounds like a plan. This series should be good to merge, then.
On Fri, 2020-10-02 at 08:09 -0700, Jakub Kicinski wrote: > On Fri, 02 Oct 2020 17:04:11 +0200 Johannes Berg wrote: > > > > Yeah, that'd work. I'd probably wonder if we shouldn't do > > > > > > > > [OP_POLICY] > > > > [OP] -> (u32, u32) > > > > > > > > in a struct with two u32's, since that's quite a bit more compact. > > > > > > What do we do if the op doesn't have a dump or do callback? > > > 0 is a valid policy ID, sadly :( > > > > Hm, good point. We could do -1 since that can't ever be reached though. > > > > But compactness isn't really that necessary here anyway, so ... > > Cool, sounds like a plan. > > This series should be good to merge, then. I suppose, I thought you wanted to change it to have separate dump/do policies? Whatever you like there, I don't really care much :) But I can also change my patches later to separately advertise dump/do policies, and simply always use the same one for now. But this series does conflict with the little bugfix I also sent, could you please take a look? https://lore.kernel.org/netdev/20201002094604.480c760e3c47.I7811da1539351a26cd0e5a10b98a8842cfbc1b55@changeid/ I'm not really sure how to handle. Thanks, johannes
On Fri, 02 Oct 2020 17:13:28 +0200 Johannes Berg wrote: > On Fri, 2020-10-02 at 08:09 -0700, Jakub Kicinski wrote: > > On Fri, 02 Oct 2020 17:04:11 +0200 Johannes Berg wrote: > > > > > Yeah, that'd work. I'd probably wonder if we shouldn't do > > > > > > > > > > [OP_POLICY] > > > > > [OP] -> (u32, u32) > > > > > > > > > > in a struct with two u32's, since that's quite a bit more compact. > > > > > > > > What do we do if the op doesn't have a dump or do callback? > > > > 0 is a valid policy ID, sadly :( > > > > > > Hm, good point. We could do -1 since that can't ever be reached though. > > > > > > But compactness isn't really that necessary here anyway, so ... > > > > Cool, sounds like a plan. > > > > This series should be good to merge, then. > > I suppose, I thought you wanted to change it to have separate dump/do > policies? Whatever you like there, I don't really care much :) I just want to make the uAPI future-proof for now. At a quick look ethtool doesn't really accept any attributes but headers for GET requests. DO and DUMP are the same there so it's not a priority for me. > But I can also change my patches later to separately advertise dump/do > policies, and simply always use the same one for now. Right that was what I was thinking. Basically: if ((op.doit && nla_put_u32(skb, CTRL_whatever_DO, idx)) || (op.dumpit && nla_put_u32(skb, CTRL_whatever_DUMP, idx))) goto nla_put_failure; > But this series does conflict with the little bugfix I also sent, could > you please take a look? > > https://lore.kernel.org/netdev/20201002094604.480c760e3c47.I7811da1539351a26cd0e5a10b98a8842cfbc1b55@changeid/ > > I'm not really sure how to handle. Yeah, just noticed that one now :S Dave, are you planning a PR to Linus soon by any chance? The conflict between this series and Johannes's fix would be logically simple to resolve but not trivial :(
On Fri, 2020-10-02 at 08:25 -0700, Jakub Kicinski wrote: > > > I suppose, I thought you wanted to change it to have separate dump/do > > policies? Whatever you like there, I don't really care much :) > > I just want to make the uAPI future-proof for now. Yeah, makes sense. > At a quick look ethtool doesn't really accept any attributes but > headers for GET requests. DO and DUMP are the same there so it's > not a priority for me. OK. > > But I can also change my patches later to separately advertise dump/do > > policies, and simply always use the same one for now. > > Right that was what I was thinking. Basically: > > if ((op.doit && nla_put_u32(skb, CTRL_whatever_DO, idx)) || > (op.dumpit && nla_put_u32(skb, CTRL_whatever_DUMP, idx))) > goto nla_put_failure; Right, easy enough. > > But this series does conflict with the little bugfix I also sent, could > > you please take a look? > > > > https://lore.kernel.org/netdev/20201002094604.480c760e3c47.I7811da1539351a26cd0e5a10b98a8842cfbc1b55@changeid/ > > > > I'm not really sure how to handle. > > Yeah, just noticed that one now :S Yeah, sorry ... The conflicts indeed weren't difficult, but non-trivial unless you know what's going on in each side. I pushed them to my mac80211-next tree, in the genetlink-op-policy-export branch (not sure you saw my patches and cover letter yet :) ) I'll wait for this to get resolved and then respin my patches with the above doit/dumpit after your series is in, and will also respin the userspace side then. johannes
On Fri, Oct 02, 2020 at 08:25:17AM -0700, Jakub Kicinski wrote: > On Fri, 02 Oct 2020 17:13:28 +0200 Johannes Berg wrote: > > I suppose, I thought you wanted to change it to have separate dump/do > > policies? Whatever you like there, I don't really care much :) > > I just want to make the uAPI future-proof for now. > > At a quick look ethtool doesn't really accept any attributes but > headers for GET requests. DO and DUMP are the same there so it's > not a priority for me. This is likely to change, for -x / --show-rxfh-indir / --show-rxfh we will neeed to specify RSS context id. This is also an example where different policy for doit and dumpit would make sense. Michal
From: Jakub Kicinski <kuba@kernel.org> Date: Fri, 2 Oct 2020 08:25:17 -0700 > Dave, are you planning a PR to Linus soon by any chance? The conflict > between this series and Johannes's fix would be logically simple to > resolve but not trivial :( Let me apply Johannes's fix to both net and net-next, and then you can respin a v3 of this series on top of that.
On Fri, 2020-10-02 at 08:09 -0700, Jakub Kicinski wrote: > On Fri, 02 Oct 2020 17:04:11 +0200 Johannes Berg wrote: > > > > Yeah, that'd work. I'd probably wonder if we shouldn't do > > > > > > > > [OP_POLICY] > > > > [OP] -> (u32, u32) > > > > > > > > in a struct with two u32's, since that's quite a bit more compact. > > > > > > What do we do if the op doesn't have a dump or do callback? > > > 0 is a valid policy ID, sadly :( > > > > Hm, good point. We could do -1 since that can't ever be reached though. > > > > But compactness isn't really that necessary here anyway, so ... > > Cool, sounds like a plan. > > This series should be good to merge, then. So I'm having second thoughts on this now :) If you ask me to split the policy dump to do/dump, like we discussed above, then what you did here for "retrieve a single policy" doesn't really make any sense? Because you'd be able to do that properly only for do, or you need my patches to get both? Perhaps it would make sense if you removed patch 10 from your set, and we add it back after my patches? Or I could submit my patches right after yours, but that leaves the code between the commits doing something weird, in that it would only give you the policies but no indication of which is for do/dump? Obviously today it'd only be one, but still, from a uAPI perspective. I guess it doesn't matter too much though, we get to the state that we want to be in, just the intermediate steps won't necessarily make much sense. For now I'll respin my patches so we see how the above do/dump separating looks. johannes
On Fri, 02 Oct 2020 22:27:19 +0200 Johannes Berg wrote: > On Fri, 2020-10-02 at 08:09 -0700, Jakub Kicinski wrote: > > On Fri, 02 Oct 2020 17:04:11 +0200 Johannes Berg wrote: > > > > > Yeah, that'd work. I'd probably wonder if we shouldn't do > > > > > > > > > > [OP_POLICY] > > > > > [OP] -> (u32, u32) > > > > > > > > > > in a struct with two u32's, since that's quite a bit more compact. > > > > > > > > What do we do if the op doesn't have a dump or do callback? > > > > 0 is a valid policy ID, sadly :( > > > > > > Hm, good point. We could do -1 since that can't ever be reached though. > > > > > > But compactness isn't really that necessary here anyway, so ... > > > > Cool, sounds like a plan. > > > > This series should be good to merge, then. > > So I'm having second thoughts on this now :) > > If you ask me to split the policy dump to do/dump, like we discussed > above, then what you did here for "retrieve a single policy" doesn't > really make any sense? Because you'd be able to do that properly only > for do, or you need my patches to get both? > > Perhaps it would make sense if you removed patch 10 from your set, and > we add it back after my patches? > > Or I could submit my patches right after yours, but that leaves the code > between the commits doing something weird, in that it would only give > you the policies but no indication of which is for do/dump? Obviously > today it'd only be one, but still, from a uAPI perspective. My thinking was that until kernel actually start using separate dump policies user space can assume policy 0 is relevant. But yeah, merging your changes first would probably be best. > I guess it doesn't matter too much though, we get to the state that we > want to be in, just the intermediate steps won't necessarily make much > sense. > > For now I'll respin my patches so we see how the above do/dump > separating looks. I, OTOH, am having second thoughts about not implementing separate policies for dump right away, since Michal said he'll need them soon :) Any ideas on how to do that cleanly? At some point it will make sense to have dumps and doits in separate structures, as you said earlier, but can we have "small" and "full" ops for both? That seems like too much :/
On Fri, 2020-10-02 at 13:50 -0700, Jakub Kicinski wrote: > > My thinking was that until kernel actually start using separate dump > policies user space can assume policy 0 is relevant. But yeah, merging > your changes first would probably be best. Works for me. I have it based on yours. Just updated my branch (top commit is 4d5045adfe90), but I'll probably only actually email it out once things are a bit more settled wrt. your changes. > I, OTOH, am having second thoughts about not implementing separate > policies for dump right away, since Michal said he'll need them soon :) :) > Any ideas on how to do that cleanly? At some point it will make sense > to have dumps and doits in separate structures, as you said earlier, > but can we have "small" and "full" ops for both? That seems like too > much :/ Not sure I understand what you just wrote :) I had originally assumed dumps would be "infrequent", and so having the small ops without dumps would be worthwhile. You said it wasn't true for other users, so small ops still have .doit and .dumpit entries. Which is fine? But in the small ops anyway you don't have a policy pointer - I guess you could have two "fallbacks" (for do and dump) in the family rather than just one? Another option - though it requires some rejiggering in my new policy dump code - would be to key the lookup based on do/dump as well. Then you could have the *same* op listed twice like struct genl_ops my_ops[] = { { .cmd = SOMETHING, .doit = do_something, .policy = something_do_policy, }, { .cmd = SOMETHING, .dumpit = dump_something, .policy = something_dump_policy, }, }; That way you only pay where needed? But ultimately with large ops you already pay for the start/dump/done pointers, and you'd have that even for the extra entry with _doit_ because ... Unless we put three different kinds of ops (small, full-do, full-dump), but that gets a bit awkward too? johannes
On Fri, 2020-10-02 at 22:59 +0200, Johannes Berg wrote: > On Fri, 2020-10-02 at 13:50 -0700, Jakub Kicinski wrote: > > My thinking was that until kernel actually start using separate dump > > policies user space can assume policy 0 is relevant. But yeah, merging > > your changes first would probably be best. > > Works for me. I have it based on yours. Just updated my branch (top > commit is 4d5045adfe90), but I'll probably only actually email it out > once things are a bit more settled wrt. your changes. Forgot the link ... https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/log/?h=genetlink-op-policy-export johannes
On Fri, 02 Oct 2020 23:00:15 +0200 Johannes Berg wrote: > On Fri, 2020-10-02 at 22:59 +0200, Johannes Berg wrote: > > On Fri, 2020-10-02 at 13:50 -0700, Jakub Kicinski wrote: > > > My thinking was that until kernel actually start using separate dump > > > policies user space can assume policy 0 is relevant. But yeah, merging > > > your changes first would probably be best. > > > > Works for me. I have it based on yours. Just updated my branch (top > > commit is 4d5045adfe90), but I'll probably only actually email it out > > once things are a bit more settled wrt. your changes. > > Forgot the link ... > > https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/log/?h=genetlink-op-policy-export If it's not too late for you - do you want to merge the two series and post everything together? Perhaps squashing patch 10 into something if that makes sense? You already seem to have it rebased.
On Fri, 2 Oct 2020 14:17:01 -0700 Jakub Kicinski wrote: > On Fri, 02 Oct 2020 23:00:15 +0200 Johannes Berg wrote: > > On Fri, 2020-10-02 at 22:59 +0200, Johannes Berg wrote: > > > On Fri, 2020-10-02 at 13:50 -0700, Jakub Kicinski wrote: > > > > My thinking was that until kernel actually start using separate dump > > > > policies user space can assume policy 0 is relevant. But yeah, merging > > > > your changes first would probably be best. > > > > > > Works for me. I have it based on yours. Just updated my branch (top > > > commit is 4d5045adfe90), but I'll probably only actually email it out > > > once things are a bit more settled wrt. your changes. > > > > Forgot the link ... > > > > https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/log/?h=genetlink-op-policy-export > > If it's not too late for you - do you want to merge the two series and > post everything together? Perhaps squashing patch 10 into something if > that makes sense? > > You already seem to have it rebased. FWIW earlier I said: if ((op.doit && nla_put_u32(skb, CTRL_whatever_DO, idx)) || (op.dumpit && nla_put_u32(skb, CTRL_whatever_DUMP, idx))) goto nla_put_failure; - we should probably also check GENL_DONT_VALIDATE_DUMP here?
On Fri, 2 Oct 2020 14:22:05 -0700 Jakub Kicinski wrote: > > > Forgot the link ... > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/log/?h=genetlink-op-policy-export > > > > If it's not too late for you - do you want to merge the two series and > > post everything together? Perhaps squashing patch 10 into something if > > that makes sense? > > > > You already seem to have it rebased. After double checking your tree has @policy in the wrong kdoc. Let me post the first 9 patches and please squash patch 10 into your series. > FWIW earlier I said: > > if ((op.doit && nla_put_u32(skb, CTRL_whatever_DO, idx)) || > (op.dumpit && nla_put_u32(skb, CTRL_whatever_DUMP, idx))) > goto nla_put_failure; > > - we should probably also check GENL_DONT_VALIDATE_DUMP here?