RE: [PATCH 1/1] net sched: Added the TC_LINKLAYER_CUSTOM linklayer type

[David Laight]

From: McCabe, Robert J
> Sent: 04 July 2017 01:14
> + if (nla_put(skb, TCA_STAB_DATA, sizeof(stab->szopts)*sizeof(u16), 
> >data))
> + goto nla_put_failure;

Multiplying sizeof(a) by sizeof(b) really doesn't look right at all.

David

Re: [PATCH 1/1] net sched: Added the TC_LINKLAYER_CUSTOM linklayer type

[Jiri Pirko]

Tue, Jul 04, 2017 at 07:12:47PM CEST, robert.mcc...@rockwellcollins.com wrote:
>>>Even so, the kernel patch you sent does not make any sense. Introduces
>>>TC_LINKLAYER_CUSTOM but does not use it.
>
>I needed to add this to the tc_link_layer enum in my iproute2 patch
>(https://patchwork.ozlabs.org/patch/784165/)
>
>enum tc_link_layer {
>  TC_LINKLAYER_UNAWARE, /* Indicate unaware old iproute2 util */
>  TC_LINKLAYER_ETHERNET,
>  TC_LINKLAYER_ATM,
>   + TC_LINKLAYER_CUSTOM,
> };
>
>I originally wasn't aware that this file was shared with the kernel --
>my original patch received the comment from Eric Dumazet
>
>You can not do this : This file is coming from the kernel
> ( include/uapi/linux/pkt_sched.h )

Correct.


>
>So I opted to make the corresponding change to the pkt_sched.h file in
>the kernel.

I'm not sure how should I put this, but you are adding enum value and
you are not using it in kernel! That is wrong. Whenever you add new
value to UAPI, it has to have a point. Please see:

$ git grep TC_LINKLAYER_


>But looking at it further, I am confused as to why this tc_link_layer even 
>needs
>to exist in the kernel anyway,  It is only used in net/sched for calculating 
>the
>size (via stab) and rate (via rtab) translations; however, these
>translations are
>already specified with in the form of the stab and rtab themselves (these are
>calculated in userspace).
>
>I think -- in a perfect world -- the tc_link_layer (and the associated
>tc_ratespec.linklayer and  tc_sizespec.linklayer) should be removed
>from the kernel,
>but I'm not sure of the compatibility implications of this ...
>Do you have any suggestions?
>
>>
>> Also, please make you email working. Says to me:
>>
>> ** Address not found **
>>
>> Your message wasn't delivered to mcc...@rockwellcollins.com because the 
>> address couldn't be found. Check for typos or unnecessary spaces and try 
>> again.
>>
>> This is annoying.
>
>I think I fixed it -- was mis-configuration in my .gitconfig (sorry,
>I'm still new at this).

Re: [PATCH 1/1] net sched: Added the TC_LINKLAYER_CUSTOM linklayer type

[Robert McCabe]

>>Even so, the kernel patch you sent does not make any sense. Introduces
>>TC_LINKLAYER_CUSTOM but does not use it.

I needed to add this to the tc_link_layer enum in my iproute2 patch
(https://patchwork.ozlabs.org/patch/784165/)

enum tc_link_layer {
  TC_LINKLAYER_UNAWARE, /* Indicate unaware old iproute2 util */
  TC_LINKLAYER_ETHERNET,
  TC_LINKLAYER_ATM,
   + TC_LINKLAYER_CUSTOM,
 };

I originally wasn't aware that this file was shared with the kernel --
my original patch received the comment from Eric Dumazet

You can not do this : This file is coming from the kernel
 ( include/uapi/linux/pkt_sched.h )

So I opted to make the corresponding change to the pkt_sched.h file in
the kernel.
But looking at it further, I am confused as to why this tc_link_layer even needs
to exist in the kernel anyway,  It is only used in net/sched for calculating the
size (via stab) and rate (via rtab) translations; however, these
translations are
already specified with in the form of the stab and rtab themselves (these are
calculated in userspace).

I think -- in a perfect world -- the tc_link_layer (and the associated
tc_ratespec.linklayer and  tc_sizespec.linklayer) should be removed
from the kernel,
but I'm not sure of the compatibility implications of this ...
Do you have any suggestions?

>
> Also, please make you email working. Says to me:
>
> ** Address not found **
>
> Your message wasn't delivered to mcc...@rockwellcollins.com because the 
> address couldn't be found. Check for typos or unnecessary spaces and try 
> again.
>
> This is annoying.

I think I fixed it -- was mis-configuration in my .gitconfig (sorry,
I'm still new at this).

Re: [PATCH 1/1] net sched: Added the TC_LINKLAYER_CUSTOM linklayer type

[Jiri Pirko]

Tue, Jul 04, 2017 at 05:34:50PM CEST, j...@resnulli.us wrote:
>Tue, Jul 04, 2017 at 04:49:24PM CEST, robert.mcc...@rockwellcollins.com wrote:
>>> You dump stab->data to user. How is this related to TC_LINKLAYER_CUSTOM
>>> and howcome this "is to support user-space modification of the qdisc
>>> stab" as your description says? I'm confused...
>>
>>I have a related patch for iproute2 where the "custom" link-layer is
>>used to allow
>>user modification of the qdisc stab.  Dumping stab->data to the user
>>is used in the
>>iproute2 patch to show the current size translation table.
>
>Even so, the kernel patch you sent does not make any sense. Introduces
>TC_LINKLAYER_CUSTOM but does not use it.

Also, please make you email working. Says to me:

** Address not found **

Your message wasn't delivered to mcc...@rockwellcollins.com because the address 
couldn't be found. Check for typos or unnecessary spaces and try again.

This is annoying.

Re: [PATCH 1/1] net sched: Added the TC_LINKLAYER_CUSTOM linklayer type

[Jiri Pirko]

Tue, Jul 04, 2017 at 04:49:24PM CEST, robert.mcc...@rockwellcollins.com wrote:
>> You dump stab->data to user. How is this related to TC_LINKLAYER_CUSTOM
>> and howcome this "is to support user-space modification of the qdisc
>> stab" as your description says? I'm confused...
>
>I have a related patch for iproute2 where the "custom" link-layer is
>used to allow
>user modification of the qdisc stab.  Dumping stab->data to the user
>is used in the
>iproute2 patch to show the current size translation table.

Even so, the kernel patch you sent does not make any sense. Introduces
TC_LINKLAYER_CUSTOM but does not use it.

Re: [PATCH 1/1] net sched: Added the TC_LINKLAYER_CUSTOM linklayer type

[Robert McCabe]

> You dump stab->data to user. How is this related to TC_LINKLAYER_CUSTOM
> and howcome this "is to support user-space modification of the qdisc
> stab" as your description says? I'm confused...

I have a related patch for iproute2 where the "custom" link-layer is
used to allow
user modification of the qdisc stab.  Dumping stab->data to the user
is used in the
iproute2 patch to show the current size translation table.

Re: [PATCH 1/1] net sched: Added the TC_LINKLAYER_CUSTOM linklayer type

[Jiri Pirko]

Tue, Jul 04, 2017 at 02:14:25AM CEST, robert.mcc...@rockwellcollins.com wrote:
>This is to support user-space modification of the qdisc stab.
>
>Signed-off-by: McCabe, Robert J 
>---
> include/uapi/linux/pkt_sched.h | 1 +
> net/sched/sch_api.c| 2 ++
> 2 files changed, 3 insertions(+)
>
>diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
>index 099bf55..289bb81 100644
>--- a/include/uapi/linux/pkt_sched.h
>+++ b/include/uapi/linux/pkt_sched.h
>@@ -82,6 +82,7 @@ enum tc_link_layer {
>   TC_LINKLAYER_UNAWARE, /* Indicate unaware old iproute2 util */
>   TC_LINKLAYER_ETHERNET,
>   TC_LINKLAYER_ATM,
>+  TC_LINKLAYER_CUSTOM,
> };
> #define TC_LINKLAYER_MASK 0x0F /* limit use to lower 4 bits */
> 
>diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>index 43b94c7..174a925 100644
>--- a/net/sched/sch_api.c
>+++ b/net/sched/sch_api.c
>@@ -533,6 +533,8 @@ static int qdisc_dump_stab(struct sk_buff *skb, struct 
>qdisc_size_table *stab)
>   goto nla_put_failure;
>   if (nla_put(skb, TCA_STAB_BASE, sizeof(stab->szopts), >szopts))
>   goto nla_put_failure;
>+  if (nla_put(skb, TCA_STAB_DATA, sizeof(stab->szopts)*sizeof(u16), 
>>data))
>+  goto nla_put_failure;
 
You dump stab->data to user. How is this related to TC_LINKLAYER_CUSTOM
and howcome this "is to support user-space modification of the qdisc
stab" as your description says? I'm confused...



>   nla_nest_end(skb, nest);
> 
>   return skb->len;
>-- 
>2.7.4
>

[PATCH 1/1] net sched: Added the TC_LINKLAYER_CUSTOM linklayer type

[McCabe, Robert J]

This is to support user-space modification of the qdisc stab.

Signed-off-by: McCabe, Robert J 
---
 include/uapi/linux/pkt_sched.h | 1 +
 net/sched/sch_api.c| 2 ++
 2 files changed, 3 insertions(+)

diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 099bf55..289bb81 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -82,6 +82,7 @@ enum tc_link_layer {
TC_LINKLAYER_UNAWARE, /* Indicate unaware old iproute2 util */
TC_LINKLAYER_ETHERNET,
TC_LINKLAYER_ATM,
+   TC_LINKLAYER_CUSTOM,
 };
 #define TC_LINKLAYER_MASK 0x0F /* limit use to lower 4 bits */
 
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 43b94c7..174a925 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -533,6 +533,8 @@ static int qdisc_dump_stab(struct sk_buff *skb, struct 
qdisc_size_table *stab)
goto nla_put_failure;
if (nla_put(skb, TCA_STAB_BASE, sizeof(stab->szopts), >szopts))
goto nla_put_failure;
+   if (nla_put(skb, TCA_STAB_DATA, sizeof(stab->szopts)*sizeof(u16), 
>data))
+   goto nla_put_failure;
nla_nest_end(skb, nest);
 
return skb->len;
-- 
2.7.4