[net-next,v4,1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch

Message ID 20170420113540.GC1886@nanopsycho.orion
State RFC
Delegated to: David Miller
Headers show

Commit Message

Jiri Pirko April 20, 2017, 11:35 a.m.
Thu, Apr 20, 2017 at 12:42:55PM CEST, jhs@mojatatu.com wrote:
>On 17-04-19 12:17 PM, Jiri Pirko wrote:
>> Wed, Apr 19, 2017 at 06:07:48PM CEST, jhs@mojatatu.com wrote:
>> > On 17-04-19 11:54 AM, Jiri Pirko wrote:
>> > > Wed, Apr 19, 2017 at 05:37:15PM CEST, jhs@mojatatu.com wrote:
>> > > > On 17-04-19 09:13 AM, Jiri Pirko wrote:
>> > > > > Wed, Apr 19, 2017 at 03:03:59PM CEST, jhs@mojatatu.com wrote:
>> > > > > > On 17-04-19 08:36 AM, Jiri Pirko wrote:
>> > > > > > > Wed, Apr 19, 2017 at 01:57:29PM CEST, jhs@mojatatu.com wrote:
>> > > > > > > > From: Jamal Hadi Salim <jhs@mojatatu.com>
>> > > > > > 
>> > > > > > > > include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++--
>> > > > > > > > net/sched/act_api.c            | 43 ++++++++++++++++++++++++++++++++----------
>> > > > > > > > 3 files changed, 53 insertions(+), 12 deletions(-)
>> > > > 
>> > > > > > > > +#define TCAA_MAX (__TCAA_MAX - 1)
>> > > > > > > > #define TA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
>> > > > > > > > #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>> > > > > > > > -#define TCA_ACT_TAB 1 /* attr type must be >=1 */	
>> > > > > > > > -#define TCAA_MAX 1
>> > > > > > > > +#define TCA_ACT_TAB TCAA_ACT_TAB
>> > > > > > > 
>> > > > > > > This is mess. What does "TCAA" stand for?
>> > > > > > 
>> > > > > > TC Actions Attributes.  What would you call it? I could have
>> > > > > > called it TCA_ROOT etc. But maybe a comment to just call it
>> > > > > > TC Actions Attributes would be enough?
>> > > > > 
>> > > > > TCA_DUMP_X
>> > > > > 
>> > > > > it is only for dumping. Naming it "attribute" seems weird. Same as if
>> > > > > you have: int variable_something;
>> > > > > 
>> > > > 
>> > > > Jiri, this is not just for dumping. We are describing high level
>> > > > attributes for tc actions.
>> > > 
>> > > This is already present:
>> > > enum {
>> > >         TCA_ACT_UNSPEC,
>> > >         TCA_ACT_KIND,
>> > >         TCA_ACT_OPTIONS,
>> > >         TCA_ACT_INDEX,
>> > >         TCA_ACT_STATS,
>> > >         TCA_ACT_PAD,
>> > >         TCA_ACT_COOKIE,
>> > >         __TCA_ACT_MAX
>> > > };
>> > > 
>> > > This is nested inside the dump message (TCAA_MAX, TCA_ACT_TAB)
>> > > 
>> > > Looks like you are mixing these 2.
>> > > 
>> > 
>> > No - That space is per action. The space i am defining is
>> > above that in the the hierarchy. There used to be only
>> > one attribute there (TCA_ACT_TAB) and now we are making
>> > it more generic.
>> 
>> Right. That's what I say. And that higher level is used only for
>> dumping. That's why I suggested TCA_ACT_DUMP prefix.
>> 
>
>DUMP is not right. _TAB is used for SET/DEL as well.
>why dont we leave this alone so we can progress?
>You can submit changes later if it bothers you still.

Ha. So the current code is wrong, I see it now. Following is needed:


You confused me squashing the change to this patch.

Ok, so the name could be:
TCA_ACTS_*
?
I believe it is crucial to figure this out right now. TC UAPI is in deep
mess already, no need to push it even more.


>
>> 
>> > 
>> > > 
>> > > 
>> > 
>> > > > It is a _lot_ of code to change! Note:
>> > > > This is all the UAPI visible code (the same coding style for 20 years).
>> > > > I am worried about this part.
>> > > 
>> > > We'll see. Lets do it in a sensitive way, in steps. But for new things,
>> > > I think it is good not to stick with old and outlived habits.
>> > > 
>> > > 
>> > 
>> > I know you have the muscle to get it done - so fine, i will start
>> > with this one change.
>> > 
>> > > 
>> > > Netlink is TLV, should be used as TLV. I don't see how you can run out
>> > > any space. You tend to use Netlink in some weird hybrid mode, with only
>> > > argument being space. I think that couple of bytes wasted is not
>> > > a problem at all...
>> > > 
>> > 
>> > You are not making sense to me still.
>> > What you describe as "a couple of bytes" adds up when you have
>> > a large number of objects. I am trying to cut down on data back
>> > and forth from user/kernel and a bitmap is a well understood entity.
>> 
>> The attributes in question are per-message, not per-object
>> 
>> 
>> > 
>> > Even if i did use a TLV - when i am representing flags which require one
>> > bit each - it makes sense to use a bitmask encapsulated in a TLV. Not
>> > to waste a TLV per bit.
>> 
>> That is the only correct way. For example, in future the kernel may
>> report in extended ack that it does not support some specific attribute
>> user passed. If you pack it all in one attr, that would not be possible.
>> Also, what prevents user from passing random data on bit flag positions
>> that you are not using? Current kernel would ignore it. Next kernel will
>> do something different according to the flag bit. That is UAPI break.
>> Essentialy the same thing what DaveM said about the struct. You have to
>> define this completely, at the beginning, not possible to use the unused
>> bits for other things in the future.
>> 
>
>
>They are not the same issue Jiri. We have used bitmasks fine on netlink

Howcome they are not the same? I'm pretty certain they are.


>message for a millenia. Nobody sets garbage on a bitmask they are not
>supposed to touch. The struct padding thing is a shame the way it
>turned out - now netlink can no longer have a claim to be a (good)
>wire protocol.
>Other thing: I know you feel strongly about this but I dont agree that
>everything has to be a TLV and in no way, as a matter of principle, you are
>going to convince me  (even when the cows come home) that I have to
>use 64 bits to carry a single bit just so I can use TLVs.

Then I guess we have to agree to disagree. I believe that your approach
is wrong and will break users in future when you add even a single flag.
Argument that "we are doing this for ages-therefore it is correct" has
simply 0 value.

Comments

Jamal Hadi Salim April 20, 2017, 2:03 p.m. | #1
On 17-04-20 07:35 AM, Jiri Pirko wrote:
> Thu, Apr 20, 2017 at 12:42:55PM CEST, jhs@mojatatu.com wrote:
>> On 17-04-19 12:17 PM, Jiri Pirko wrote:
>
> Ha. So the current code is wrong, I see it now. Following is needed:
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 82b1d48..c432b22 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1005,7 +1005,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
>  	    !netlink_capable(skb, CAP_NET_ADMIN))
>  		return -EPERM;
>
> -	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
> +	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, NULL,
>  			  extack);
>  	if (ret < 0)
>  		return ret;
>
> You confused me squashing the change to this patch.
>
> Ok, so the name could be:
> TCA_ACTS_*
> ?
> I believe it is crucial to figure this out right now. TC UAPI is in deep
> mess already, no need to push it even more.
>

I could change that name. Look at the last series i sent, add any extra
comments to that  and i will get to it by tomorrow.

>> They are not the same issue Jiri. We have used bitmasks fine on netlink
>
> Howcome they are not the same? I'm pretty certain they are.
>

They are not Jiri. I challenge you to point me to one netlink bitmask
representation used that has some bits that were not being used that
someone or some compiler is going to set some random values on.
Typically flags are u64/32/16

>
>> message for a millenia. Nobody sets garbage on a bitmask they are not
>> supposed to touch. The struct padding thing is a shame the way it
>> turned out - now netlink can no longer have a claim to be a (good)
>> wire protocol.
>> Other thing: I know you feel strongly about this but I dont agree that
>> everything has to be a TLV and in no way, as a matter of principle, you are
>> going to convince me  (even when the cows come home) that I have to
>> use 64 bits to carry a single bit just so I can use TLVs.
>
> Then I guess we have to agree to disagree. I believe that your approach
> is wrong and will break users in future when you add even a single flag.
> Argument that "we are doing this for ages-therefore it is correct" has
> simply 0 value.
>

"doing these for 80 years" is not the main driving point.
I appreciate that data structures - even when fields are
annotated as "pads" there is no guarantee when i allocate one
the pads will be zeroes.

Bitmasks are atomic in nature and therefore used in whole - not complex
like structs. Thats the difference. I dont define this u32 bitmap that
has 8 bits that are "pads" because that concept doesnt make sense for
atomic fields.
This implies that 6 months down the road I dont regret and say "shit, I
need 2 more bits for flagbits but some idiot or compiler was setting
those fields so i cant use them now because the kernel wont tell the
difference between what s/he is doing and real values".
Allocations of data structures I can understand, unless we enforce
always reset to zeroes all the struct members as part of creation.

But lets agree to disagree if you are still not convinced.

On your "everything must be a TLV". Caveat is: there is a balance
between  performance and flexibility. I should be able to pack aligned
complex data structures in a TLV for performance reasons.
One lesson is: in the future we can make these extra service header like
tcamsg very small and always use all their fields instead of defining
pads.

cheers,
jamal

Patch

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 82b1d48..c432b22 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1005,7 +1005,7 @@  static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 	    !netlink_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
-	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
+	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, NULL,
 			  extack);
 	if (ret < 0)
 		return ret;