Message-ID: <20201001092434.3f916d80@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 1 Oct 2020 09:24:34 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     netdev@...r.kernel.org, andrew@...n.ch, jiri@...nulli.us,
        mkubecek@...e.cz, dsahern@...nel.org, pablo@...filter.org
Subject: Re: [RFC net-next 9/9] genetlink: allow dumping command-specific
 policy

On Thu, 01 Oct 2020 18:00:58 +0200 Johannes Berg wrote:
> On Thu, 2020-10-01 at 08:41 -0700, Jakub Kicinski wrote:
> 
> > > Even if I don't have a good idea now on how to avoid the duplication, it
> > > might be nicer to have a (flag) attribute here for "CTRL_ATTR_ALL_OPS"?  
> > 
> > Hm. How would you see the dump structured?   
> 
> Yeah, that's the tricky part ... Hence why I said "I don't have a good
> idea now" :)

You say that, yet your idea below seems pretty good :P

> > We need to annotate the root
> > policies with the command. Right now I have:
> > 
> >  [ATTR_FAMILY_ID]
> >  [ATTR_OP]
> >  [ATTR_POLICY]
> >    [policy idx]
> >      [attr idx]
> >        [bla]
> >        [bla]
> >        [bla]
> > 
> > But if we're dumping _all_ the policy to op mapping is actually 1:n,
> > so we'd need to restructure the dump a lil' bit and have OP only
> > reported on root of the policy and make it a nested array.  
> 
> So today you see something like
> 
> [ATTR_FAMILY_ID]
> [ATTR_POLICY]
>   [policy idx, 0 = main policy]
>     [bla]
>     ...
>   ...
> 
> 
> I guess the most compact representation, that also preserves the most
> data about sharing, would be to do something like
> 
> [ATTR_FAMILY_ID]
> [ATTR_POLICY]
>   [policy idx, 0 = main policy]
>     [bla]
>     ...
>   ...
> [ATTR_OP_POLICY]
>   [op] = [policy idx]
>   ...
> 
> This preserves all the information because it tells you which policies
> actually are identical (shared), each per-op policy can have nested
> policies referring to common ones, like in the ethtool case, etc.

Only comment I have is - can we make sure to put the ATTR_OP_POLICY
first? That way user space can parse the stream an pick out the info
it needs rather than recording all the policies only to find out later
which one is which.

> OTOH, it's a lot trickier to implement - I haven't really come up with a
> good way of doing it "generically" with the net/netlink/policy.c code.
> I'm sure it can be solved, but I haven't really given it enough thought.
> Perhaps by passing a "policy iterator" to netlink_policy_dump_start(),
> instead of just a single policy (i.e. a function & a data ptr or so),
> and then it can walk all the policies using that, assign the idxes etc.,
> and dump them out in netlink_policy_dump_write()?
> 
> But then we'd still have to get the policy idx for a given policy, and
> not clean up all the state when netlink_policy_dump_loop() returns
> false, because you still need it for ATTR_OP_POLICY to find the idx from
> the pointer?
> 
> I guess it's doable. Just seems a bit more complex. OTOH, it may be that
> such complexity also completely makes sense for non-generic netlink
> families anyway, I haven't looked at them much at all.

IDK, doesn't seem crazy hard. We can create some iterator or expand the
API with "begin" "add" "end" calls. Then once dumper state is build we
can ask it which ids it assigned.

OTOH I don't think we have a use for this in ethtool, because user
space usually does just one op per execution. So I'm thinking to use
your structure for the dump, but leave the actual implementation of
"dump all" for "later".

How does that sound?

Powered by blists - more mailing lists