Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod
On Thu, May 24, 2018 at 10:45 PM, Fu, Qiaobin wrote: > The new action inheritdsfield copies the field DS of > IPv4 and IPv6 packets into skb->priority. This enables > later classification of packets based on the DS field. Please move it to skbedit.
Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod
On Fri, May 25, 2018 at 05:45:03AM +, Fu, Qiaobin wrote:
> Hi Marcelo,
>
> Thanks for pointing out these style issues. Below is the updated version:
Hi Qiaobin,
Looks good to me. Now you have to submit it like you submitted the
original patch, but add the version tag to the summary. Like '[PATCH
v2 net-next] '
And without the text before the changelog.
Thanks.
>
> ---
> The new action inheritdsfield copies the field DS of
> IPv4 and IPv6 packets into skb->priority. This enables
> later classification of packets based on the DS field.
>
> Original idea by Jamal Hadi Salim
>
> Signed-off-by: Qiaobin Fu
> Reviewed-by: Michel Machado
> ---
>
> Note that the motivation for this patch is found in the following discussion:
> https://www.spinics.net/lists/netdev/msg501061.html
> ---
>
> diff --git a/include/uapi/linux/tc_act/tc_skbmod.h
> b/include/uapi/linux/tc_act/tc_skbmod.h
> index 38c072f..0718b48 100644
> --- a/include/uapi/linux/tc_act/tc_skbmod.h
> +++ b/include/uapi/linux/tc_act/tc_skbmod.h
> @@ -19,6 +19,7 @@
> #define SKBMOD_F_SMAC0x2
> #define SKBMOD_F_ETYPE 0x4
> #define SKBMOD_F_SWAPMAC 0x8
> +#define SKBMOD_F_INHERITDSFIELD 0x10
>
> struct tc_skbmod {
> tc_gen;
> diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
> index ad050d7..e2082f6 100644
> --- a/net/sched/act_skbmod.c
> +++ b/net/sched/act_skbmod.c
> @@ -16,6 +16,9 @@
> #include
> #include
> #include
> +#include
> +#include
> +#include
>
> #include
> #include
> @@ -72,6 +75,26 @@ static int tcf_skbmod_run(struct sk_buff *skb, const
> struct tc_action *a,
> ether_addr_copy(eth_hdr(skb)->h_source, (u8 *)tmpaddr);
> }
>
> + if (flags & SKBMOD_F_INHERITDSFIELD) {
> + int wlen = skb_network_offset(skb);
> +
> + switch (tc_skb_protocol(skb)) {
> + case htons(ETH_P_IP):
> + wlen += sizeof(struct iphdr);
> + if (!pskb_may_pull(skb, wlen))
> + return TC_ACT_SHOT;
> + skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
> + break;
> +
> + case htons(ETH_P_IPV6):
> + wlen += sizeof(struct ipv6hdr);
> + if (!pskb_may_pull(skb, wlen))
> + return TC_ACT_SHOT;
> + skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
> + break;
> + }
> + }
> +
> return action;
> }
>
> @@ -127,6 +150,9 @@ static int tcf_skbmod_init(struct net *net, struct nlattr
> *nla,
> if (parm->flags & SKBMOD_F_SWAPMAC)
> lflags = SKBMOD_F_SWAPMAC;
>
> + if (parm->flags & SKBMOD_F_INHERITDSFIELD)
> + lflags |= SKBMOD_F_INHERITDSFIELD;
> +
> exists = tcf_idr_check(tn, parm->index, a, bind);
> if (exists && bind)
> return 0;
>
> > On May 23, 2018, at 2:06 PM, Marcelo Ricardo Leitner
> > wrote:
> >
> > Hi,
> >
> > Some style fixes:
> >
> > On Thu, May 17, 2018 at 07:33:08PM +, Fu, Qiaobin wrote:
> >> net/sched: add action inheritdsfield to skbmod
> >
> > This extra line above should not be here.
> >
> >>
> >> The new action inheritdsfield copies the field DS of
> >> IPv4 and IPv6 packets into skb->prioriry. This enables
> > typo -^
> >
> >> later classification of packets based on the DS field.
> >>
> >> Original idea by Jamal Hadi Salim
> >>
> >> Signed-off-by: Qiaobin Fu
> >> Reviewed-by: Michel Machado
> >> ---
> >>
> >> Note that the motivation for this patch is found in the following
> >> discussion:
> >> https://www.spinics.net/lists/netdev/msg501061.html
> >> ---
> >>
> >> diff --git a/include/uapi/linux/tc_act/tc_skbmod.h
> >> b/include/uapi/linux/tc_act/tc_skbmod.h
> >> index 38c072f..0718b48 100644
> >> --- a/include/uapi/linux/tc_act/tc_skbmod.h
> >> +++ b/include/uapi/linux/tc_act/tc_skbmod.h
> >> @@ -19,6 +19,7 @@
> >> #define SKBMOD_F_SMAC 0x2
> >> #define SKBMOD_F_ETYPE 0x4
> >> #define SKBMOD_F_SWAPMAC 0x8
> >> +#define SKBMOD_F_INHERITDSFIELD 0x10
> >>
> >> struct tc_skbmod {
> >>tc_gen;
> >> diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
> >> index ad050d7..21d5bec 100644
> >> --- a/net/sched/act_skbmod.c
> >> +++ b/net/sched/act_skbmod.c
> >> @@ -16,6 +16,9 @@
> >> #include
> >> #include
> >> #include
> >> +#include
> >> +#include
> >> +#include
> >>
> >> #include
> >> #include
> >> @@ -72,6 +75,25 @@ static int tcf_skbmod_run(struct sk_buff *skb, const
> >> struct tc_action *a,
> >>ether_addr_copy(eth_hdr(skb)->h_source, (u8 *)tmpaddr);
> >>}
> >>
> >> + if (flags & SKBMOD_F_INHERITDSFIELD) {
> >> + int wlen = skb_network_offset(skb);
> >
> > You need a blank line here, between var declaration and the rest.
> >
> >> + switch (tc_skb_protocol(skb)) {
> >> + case htons(ETH_P_IP):
> >> +
Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod
Hi Marcelo,
Thanks for pointing out these style issues. Below is the updated version:
---
The new action inheritdsfield copies the field DS of
IPv4 and IPv6 packets into skb->priority. This enables
later classification of packets based on the DS field.
Original idea by Jamal Hadi Salim
Signed-off-by: Qiaobin Fu
Reviewed-by: Michel Machado
---
Note that the motivation for this patch is found in the following discussion:
https://www.spinics.net/lists/netdev/msg501061.html
---
diff --git a/include/uapi/linux/tc_act/tc_skbmod.h
b/include/uapi/linux/tc_act/tc_skbmod.h
index 38c072f..0718b48 100644
--- a/include/uapi/linux/tc_act/tc_skbmod.h
+++ b/include/uapi/linux/tc_act/tc_skbmod.h
@@ -19,6 +19,7 @@
#define SKBMOD_F_SMAC 0x2
#define SKBMOD_F_ETYPE 0x4
#define SKBMOD_F_SWAPMAC 0x8
+#define SKBMOD_F_INHERITDSFIELD 0x10
struct tc_skbmod {
tc_gen;
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index ad050d7..e2082f6 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -16,6 +16,9 @@
#include
#include
#include
+#include
+#include
+#include
#include
#include
@@ -72,6 +75,26 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct
tc_action *a,
ether_addr_copy(eth_hdr(skb)->h_source, (u8 *)tmpaddr);
}
+ if (flags & SKBMOD_F_INHERITDSFIELD) {
+ int wlen = skb_network_offset(skb);
+
+ switch (tc_skb_protocol(skb)) {
+ case htons(ETH_P_IP):
+ wlen += sizeof(struct iphdr);
+ if (!pskb_may_pull(skb, wlen))
+ return TC_ACT_SHOT;
+ skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
+ break;
+
+ case htons(ETH_P_IPV6):
+ wlen += sizeof(struct ipv6hdr);
+ if (!pskb_may_pull(skb, wlen))
+ return TC_ACT_SHOT;
+ skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
+ break;
+ }
+ }
+
return action;
}
@@ -127,6 +150,9 @@ static int tcf_skbmod_init(struct net *net, struct nlattr
*nla,
if (parm->flags & SKBMOD_F_SWAPMAC)
lflags = SKBMOD_F_SWAPMAC;
+ if (parm->flags & SKBMOD_F_INHERITDSFIELD)
+ lflags |= SKBMOD_F_INHERITDSFIELD;
+
exists = tcf_idr_check(tn, parm->index, a, bind);
if (exists && bind)
return 0;
> On May 23, 2018, at 2:06 PM, Marcelo Ricardo Leitner
> wrote:
>
> Hi,
>
> Some style fixes:
>
> On Thu, May 17, 2018 at 07:33:08PM +, Fu, Qiaobin wrote:
>> net/sched: add action inheritdsfield to skbmod
>
> This extra line above should not be here.
>
>>
>> The new action inheritdsfield copies the field DS of
>> IPv4 and IPv6 packets into skb->prioriry. This enables
> typo -^
>
>> later classification of packets based on the DS field.
>>
>> Original idea by Jamal Hadi Salim
>>
>> Signed-off-by: Qiaobin Fu
>> Reviewed-by: Michel Machado
>> ---
>>
>> Note that the motivation for this patch is found in the following discussion:
>> https://www.spinics.net/lists/netdev/msg501061.html
>> ---
>>
>> diff --git a/include/uapi/linux/tc_act/tc_skbmod.h
>> b/include/uapi/linux/tc_act/tc_skbmod.h
>> index 38c072f..0718b48 100644
>> --- a/include/uapi/linux/tc_act/tc_skbmod.h
>> +++ b/include/uapi/linux/tc_act/tc_skbmod.h
>> @@ -19,6 +19,7 @@
>> #define SKBMOD_F_SMAC0x2
>> #define SKBMOD_F_ETYPE 0x4
>> #define SKBMOD_F_SWAPMAC 0x8
>> +#define SKBMOD_F_INHERITDSFIELD 0x10
>>
>> struct tc_skbmod {
>> tc_gen;
>> diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
>> index ad050d7..21d5bec 100644
>> --- a/net/sched/act_skbmod.c
>> +++ b/net/sched/act_skbmod.c
>> @@ -16,6 +16,9 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> +#include
>>
>> #include
>> #include
>> @@ -72,6 +75,25 @@ static int tcf_skbmod_run(struct sk_buff *skb, const
>> struct tc_action *a,
>> ether_addr_copy(eth_hdr(skb)->h_source, (u8 *)tmpaddr);
>> }
>>
>> +if (flags & SKBMOD_F_INHERITDSFIELD) {
>> +int wlen = skb_network_offset(skb);
>
> You need a blank line here, between var declaration and the rest.
>
>> +switch (tc_skb_protocol(skb)) {
>> +case htons(ETH_P_IP):
>> +wlen += sizeof(struct iphdr);
>> +if (!pskb_may_pull(skb, wlen))
>> +return TC_ACT_SHOT;
>> +skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
>> +break;
>> +
>> +case htons(ETH_P_IPV6):
>> +wlen += sizeof(struct ipv6hdr);
>> +if (!pskb_may_pull(skb, wlen))
>> +return TC_ACT_SHOT;
>> +skb->priority = ipv6_get_dsfield(ipv6_hdr(skb
Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod
On 23/05/18 07:01 PM, Cong Wang wrote: On Thu, May 17, 2018 at 12:33 PM, Fu, Qiaobin wrote: Hmm, but skbedit seems better than skbmod for this job, given: 1) It already modifies skb->priority, although with a given value 2) skbmod doesn't change skb metadata, it only changes payload I am _not_ saying there is strict rule for what skbmod can or can't change, it calls itself "data modifier", so I am saying we probably need to follow this existing practice. I am indifferent - you can move it to skbedit. Note: I have patches which i will send out at some point when I get the chance on pedit for implementing the concept of "copy data to metadata" and "copy metadata to data" for pedit it made a lot of sense to add the feature there. In this case skbedit makes more sense. cheers, jamal
Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod
On Thu, May 17, 2018 at 12:33 PM, Fu, Qiaobin wrote: > net/sched: add action inheritdsfield to skbmod > > The new action inheritdsfield copies the field DS of > IPv4 and IPv6 packets into skb->prioriry. This enables > later classification of packets based on the DS field. > > Original idea by Jamal Hadi Salim > > Signed-off-by: Qiaobin Fu > Reviewed-by: Michel Machado Hmm, but skbedit seems better than skbmod for this job, given: 1) It already modifies skb->priority, although with a given value 2) skbmod doesn't change skb metadata, it only changes payload I am _not_ saying there is strict rule for what skbmod can or can't change, it calls itself "data modifier", so I am saying we probably need to follow this existing practice.
Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod
Hi,
Some style fixes:
On Thu, May 17, 2018 at 07:33:08PM +, Fu, Qiaobin wrote:
> net/sched: add action inheritdsfield to skbmod
This extra line above should not be here.
>
> The new action inheritdsfield copies the field DS of
> IPv4 and IPv6 packets into skb->prioriry. This enables
typo -^
> later classification of packets based on the DS field.
>
> Original idea by Jamal Hadi Salim
>
> Signed-off-by: Qiaobin Fu
> Reviewed-by: Michel Machado
> ---
>
> Note that the motivation for this patch is found in the following discussion:
> https://www.spinics.net/lists/netdev/msg501061.html
> ---
>
> diff --git a/include/uapi/linux/tc_act/tc_skbmod.h
> b/include/uapi/linux/tc_act/tc_skbmod.h
> index 38c072f..0718b48 100644
> --- a/include/uapi/linux/tc_act/tc_skbmod.h
> +++ b/include/uapi/linux/tc_act/tc_skbmod.h
> @@ -19,6 +19,7 @@
> #define SKBMOD_F_SMAC0x2
> #define SKBMOD_F_ETYPE 0x4
> #define SKBMOD_F_SWAPMAC 0x8
> +#define SKBMOD_F_INHERITDSFIELD 0x10
>
> struct tc_skbmod {
> tc_gen;
> diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
> index ad050d7..21d5bec 100644
> --- a/net/sched/act_skbmod.c
> +++ b/net/sched/act_skbmod.c
> @@ -16,6 +16,9 @@
> #include
> #include
> #include
> +#include
> +#include
> +#include
>
> #include
> #include
> @@ -72,6 +75,25 @@ static int tcf_skbmod_run(struct sk_buff *skb, const
> struct tc_action *a,
> ether_addr_copy(eth_hdr(skb)->h_source, (u8 *)tmpaddr);
> }
>
> + if (flags & SKBMOD_F_INHERITDSFIELD) {
> + int wlen = skb_network_offset(skb);
You need a blank line here, between var declaration and the rest.
> + switch (tc_skb_protocol(skb)) {
> + case htons(ETH_P_IP):
> + wlen += sizeof(struct iphdr);
> + if (!pskb_may_pull(skb, wlen))
> + return TC_ACT_SHOT;
> + skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
> + break;
> +
> + case htons(ETH_P_IPV6):
> + wlen += sizeof(struct ipv6hdr);
> + if (!pskb_may_pull(skb, wlen))
> + return TC_ACT_SHOT;
> + skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
> + break;
> + }
> + }
> +
> return action;
> }
>
> @@ -127,6 +149,9 @@ static int tcf_skbmod_init(struct net *net, struct nlattr
> *nla,
> if (parm->flags & SKBMOD_F_SWAPMAC)
> lflags = SKBMOD_F_SWAPMAC;
>
> + if (parm->flags & SKBMOD_F_INHERITDSFIELD)
> + lflags |= SKBMOD_F_INHERITDSFIELD;
> +
> exists = tcf_idr_check(tn, parm->index, a, bind);
> if (exists && bind)
> return 0;
Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod
On 21/05/18 10:42 AM, Fu, Qiaobin wrote: Hi Jamal, I've tested my patch before publishing it here, and Nishanth is going to test it further with version 2 of the GKprio. I'm going to push a patch to the repository iproute2 to add support for "inheritdsfield”. Thanks. I already acked the kernel patch. It looks good on its own. Would you consider adding one or more tdc tests as well? cheers, jamal
Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod
On 17/05/18 03:33 PM, Fu, Qiaobin wrote: net/sched: add action inheritdsfield to skbmod The new action inheritdsfield copies the field DS of IPv4 and IPv6 packets into skb->prioriry. This enables later classification of packets based on the DS field. Original idea by Jamal Hadi Salim Signed-off-by: Qiaobin Fu Reviewed-by: Michel Machado LGTM. Thanks for putting the effort. As a tradition we also require that you post the iproute2 patch and make sure this works. Can you please do that? Also: If you can add at least a test on tdc it would help immensely. Other than that: Acked-by: Jamal Hadi Salim cheers, jamal
[PATCH net-next] net:sched: add action inheritdsfield to skbmod
net/sched: add action inheritdsfield to skbmod
The new action inheritdsfield copies the field DS of
IPv4 and IPv6 packets into skb->prioriry. This enables
later classification of packets based on the DS field.
Original idea by Jamal Hadi Salim
Signed-off-by: Qiaobin Fu
Reviewed-by: Michel Machado
---
Note that the motivation for this patch is found in the following discussion:
https://www.spinics.net/lists/netdev/msg501061.html
---
diff --git a/include/uapi/linux/tc_act/tc_skbmod.h
b/include/uapi/linux/tc_act/tc_skbmod.h
index 38c072f..0718b48 100644
--- a/include/uapi/linux/tc_act/tc_skbmod.h
+++ b/include/uapi/linux/tc_act/tc_skbmod.h
@@ -19,6 +19,7 @@
#define SKBMOD_F_SMAC 0x2
#define SKBMOD_F_ETYPE 0x4
#define SKBMOD_F_SWAPMAC 0x8
+#define SKBMOD_F_INHERITDSFIELD 0x10
struct tc_skbmod {
tc_gen;
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index ad050d7..21d5bec 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -16,6 +16,9 @@
#include
#include
#include
+#include
+#include
+#include
#include
#include
@@ -72,6 +75,25 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct
tc_action *a,
ether_addr_copy(eth_hdr(skb)->h_source, (u8 *)tmpaddr);
}
+ if (flags & SKBMOD_F_INHERITDSFIELD) {
+ int wlen = skb_network_offset(skb);
+ switch (tc_skb_protocol(skb)) {
+ case htons(ETH_P_IP):
+ wlen += sizeof(struct iphdr);
+ if (!pskb_may_pull(skb, wlen))
+ return TC_ACT_SHOT;
+ skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
+ break;
+
+ case htons(ETH_P_IPV6):
+ wlen += sizeof(struct ipv6hdr);
+ if (!pskb_may_pull(skb, wlen))
+ return TC_ACT_SHOT;
+ skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
+ break;
+ }
+ }
+
return action;
}
@@ -127,6 +149,9 @@ static int tcf_skbmod_init(struct net *net, struct nlattr
*nla,
if (parm->flags & SKBMOD_F_SWAPMAC)
lflags = SKBMOD_F_SWAPMAC;
+ if (parm->flags & SKBMOD_F_INHERITDSFIELD)
+ lflags |= SKBMOD_F_INHERITDSFIELD;
+
exists = tcf_idr_check(tn, parm->index, a, bind);
if (exists && bind)
return 0;
