* [PATCH] sch_htb: let skb->priority refer to non-leaf class @ 2014-01-16 14:45 Harry Mason 2014-01-16 16:25 ` Eric Dumazet 0 siblings, 1 reply; 10+ messages in thread From: Harry Mason @ 2014-01-16 14:45 UTC (permalink / raw) To: Jamal Hadi Salim; +Cc: linux-netdev If the class in skb->priority is not a leaf, apply filters from the selected class, not the qdisc. This allows netfilter or user space to partially classify the packet and tc filters to finish it off. Signed-off-by: Harry Mason <harry.mason@smoothwall.net> --- net/sched/sch_htb.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c index 717b210..e50ab65 100644 --- a/net/sched/sch_htb.c +++ b/net/sched/sch_htb.c @@ -223,7 +223,13 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch, return cl; *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; - tcf = q->filter_list; + + /* Start with inner filter chain if a non-leaf class is selected */ + if (cl) + tcf = cl->filter_list; + else + tcf = q->filter_list; + while (tcf && (result = tc_classify(skb, tcf, &res)) >= 0) { #ifdef CONFIG_NET_CLS_ACT switch (result) { -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] sch_htb: let skb->priority refer to non-leaf class 2014-01-16 14:45 [PATCH] sch_htb: let skb->priority refer to non-leaf class Harry Mason @ 2014-01-16 16:25 ` Eric Dumazet 2014-01-17 10:19 ` [PATCH v2] " Harry Mason 0 siblings, 1 reply; 10+ messages in thread From: Eric Dumazet @ 2014-01-16 16:25 UTC (permalink / raw) To: Harry Mason; +Cc: Jamal Hadi Salim, linux-netdev On Thu, 2014-01-16 at 14:45 +0000, Harry Mason wrote: > If the class in skb->priority is not a leaf, apply filters from the > selected class, not the qdisc. This allows netfilter or user space > to partially classify the packet and tc filters to finish it off. > > Signed-off-by: Harry Mason <harry.mason@smoothwall.net> > --- > net/sched/sch_htb.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c > index 717b210..e50ab65 100644 > --- a/net/sched/sch_htb.c > +++ b/net/sched/sch_htb.c > @@ -223,7 +223,13 @@ static struct htb_class *htb_classify(struct > sk_buff *skb, struct Qdisc *sch, > return cl; > > *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; > - tcf = q->filter_list; > + > + /* Start with inner filter chain if a non-leaf class is selected */ > + if (cl) > + tcf = cl->filter_list; > + else > + tcf = q->filter_list; > + Interesting idea. Could this break some existing htb setups ? Also we test cl being NULL at line 222, it would be nice to not test it again... ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] sch_htb: let skb->priority refer to non-leaf class 2014-01-16 16:25 ` Eric Dumazet @ 2014-01-17 10:19 ` Harry Mason 2014-01-17 12:53 ` Sergei Shtylyov 2014-01-17 14:35 ` [PATCH v2] " Eric Dumazet 0 siblings, 2 replies; 10+ messages in thread From: Harry Mason @ 2014-01-17 10:19 UTC (permalink / raw) To: Eric Dumazet, Jamal Hadi Salim; +Cc: linux-netdev If the class in skb->priority is not a leaf, apply filters from the selected class, not the qdisc. This lets netfilter or user space partially classify the packet. Signed-off-by: Harry Mason <harry.mason@smoothwall.net> --- On Thu, 2014-01-16 at 08:25 -0800, Eric Dumazet wrote: > On Thu, 2014-01-16 at 14:45 +0000, Harry Mason wrote: > >> + /* Start with inner filter chain if a non-leaf class is selected */ >> + if (cl) >> + tcf = cl->filter_list; >> + else >> + tcf = q->filter_list; > > Could this break some existing htb setups ? I think it is unlikely. Setting skb->priority to a non-leaf class would be equivalent to setting it to the base qdisc. In theory an application might rely on this if it expects the classes to be dynamic, but adding a filter could restore the old behaviour. To me this is intuitively how it should behave, and reproduces what would happen if a tc filter instead of netfilter had first assigned the non-leaf class. > Also we test cl being NULL at line 222, it would be nice to not > test it again... Updated below. net/sched/sch_htb.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c index 717b210..8073d92 100644 --- a/net/sched/sch_htb.c +++ b/net/sched/sch_htb.c @@ -219,11 +219,15 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch, if (skb->priority == sch->handle) return HTB_DIRECT; /* X:0 (direct flow) selected */ cl = htb_find(skb->priority, sch); - if (cl && cl->level == 0) - return cl; + if (cl) { + if (cl->level == 0) + return cl; + /* Start with inner filter chain if a non-leaf class is selected */ + tcf = cl->filter_list; + } else + tcf = q->filter_list; *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; - tcf = q->filter_list; while (tcf && (result = tc_classify(skb, tcf, &res)) >= 0) { #ifdef CONFIG_NET_CLS_ACT switch (result) { -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] sch_htb: let skb->priority refer to non-leaf class 2014-01-17 10:19 ` [PATCH v2] " Harry Mason @ 2014-01-17 12:53 ` Sergei Shtylyov 2014-01-17 13:22 ` [PATCH v3] " Harry Mason 2014-01-17 14:35 ` [PATCH v2] " Eric Dumazet 1 sibling, 1 reply; 10+ messages in thread From: Sergei Shtylyov @ 2014-01-17 12:53 UTC (permalink / raw) To: Harry Mason, Eric Dumazet, Jamal Hadi Salim; +Cc: linux-netdev Hello. On 17-01-2014 14:19, Harry Mason wrote: > If the class in skb->priority is not a leaf, apply filters from the > selected class, not the qdisc. This lets netfilter or user space > partially classify the packet. > Signed-off-by: Harry Mason <harry.mason@smoothwall.net> > --- > On Thu, 2014-01-16 at 08:25 -0800, Eric Dumazet wrote: >> On Thu, 2014-01-16 at 14:45 +0000, Harry Mason wrote: >>> + /* Start with inner filter chain if a non-leaf class is selected */ >>> + if (cl) >>> + tcf = cl->filter_list; >>> + else >>> + tcf = q->filter_list; >> Could this break some existing htb setups ? > I think it is unlikely. Setting skb->priority to a non-leaf class would > be equivalent to setting it to the base qdisc. In theory an application > might rely on this if it expects the classes to be dynamic, but adding > a filter could restore the old behaviour. > To me this is intuitively how it should behave, and reproduces what would > happen if a tc filter instead of netfilter had first assigned the > non-leaf class. >> Also we test cl being NULL at line 222, it would be nice to not >> test it again... > Updated below. > net/sched/sch_htb.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c > index 717b210..8073d92 100644 > --- a/net/sched/sch_htb.c > +++ b/net/sched/sch_htb.c > @@ -219,11 +219,15 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch, > if (skb->priority == sch->handle) > return HTB_DIRECT; /* X:0 (direct flow) selected */ > cl = htb_find(skb->priority, sch); > - if (cl && cl->level == 0) > - return cl; > + if (cl) { > + if (cl->level == 0) > + return cl; > + /* Start with inner filter chain if a non-leaf class is selected */ > + tcf = cl->filter_list; > + } else > + tcf = q->filter_list; There should be {} in the *else* arm, since it's in another arm of *if* already -- see Documentation/CodingStyle. WBR, Sergei ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] sch_htb: let skb->priority refer to non-leaf class 2014-01-17 12:53 ` Sergei Shtylyov @ 2014-01-17 13:22 ` Harry Mason 2014-01-21 22:36 ` David Miller ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Harry Mason @ 2014-01-17 13:22 UTC (permalink / raw) To: Sergei Shtylyov, Jamal Hadi Salim, Eric Dumazet; +Cc: linux-netdev If the class in skb->priority is not a leaf, apply filters from the selected class, not the qdisc. This lets netfilter or user space partially classify the packet. Signed-off-by: Harry Mason <harry.mason@smoothwall.net> --- On Fri, 2014-01-17 at 16:53 +0400, Sergei Shtylyov wrote: > On 17-01-2014 14:19, Harry Mason wrote: > > + if (cl) { > > + if (cl->level == 0) > > + return cl; > > + /* Start with inner filter chain if a non-leaf class is selected */ > > + tcf = cl->filter_list; > > + } else > > + tcf = q->filter_list; > > There should be {} in the *else* arm, since it's in another arm of *if* > already -- see Documentation/CodingStyle. Thanks, updated. net/sched/sch_htb.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c index 717b210..e9c8c08 100644 --- a/net/sched/sch_htb.c +++ b/net/sched/sch_htb.c @@ -219,11 +219,16 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch, if (skb->priority == sch->handle) return HTB_DIRECT; /* X:0 (direct flow) selected */ cl = htb_find(skb->priority, sch); - if (cl && cl->level == 0) - return cl; + if (cl) { + if (cl->level == 0) + return cl; + /* Start with inner filter chain if a non-leaf class is selected */ + tcf = cl->filter_list; + } else { + tcf = q->filter_list; + } *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; - tcf = q->filter_list; while (tcf && (result = tc_classify(skb, tcf, &res)) >= 0) { #ifdef CONFIG_NET_CLS_ACT switch (result) { -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] sch_htb: let skb->priority refer to non-leaf class 2014-01-17 13:22 ` [PATCH v3] " Harry Mason @ 2014-01-21 22:36 ` David Miller 2014-01-22 12:41 ` Jamal Hadi Salim 2014-01-22 14:47 ` Eric Dumazet 2014-01-23 1:40 ` David Miller 2 siblings, 1 reply; 10+ messages in thread From: David Miller @ 2014-01-21 22:36 UTC (permalink / raw) To: harry.mason; +Cc: sergei.shtylyov, hadi, eric.dumazet, netdev From: Harry Mason <harry.mason@smoothwall.net> Date: Fri, 17 Jan 2014 13:22:32 +0000 > If the class in skb->priority is not a leaf, apply filters from the > selected class, not the qdisc. This lets netfilter or user space > partially classify the packet. > > Signed-off-by: Harry Mason <harry.mason@smoothwall.net> Can I please get some reviews of this patch? Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] sch_htb: let skb->priority refer to non-leaf class 2014-01-21 22:36 ` David Miller @ 2014-01-22 12:41 ` Jamal Hadi Salim 0 siblings, 0 replies; 10+ messages in thread From: Jamal Hadi Salim @ 2014-01-22 12:41 UTC (permalink / raw) To: David Miller; +Cc: harry.mason, sergei.shtylyov, Eric Dumazet, netdev Looks reasonable from my view. cheers, jamal On Tue, Jan 21, 2014 at 5:36 PM, David Miller <davem@davemloft.net> wrote: > From: Harry Mason <harry.mason@smoothwall.net> > Date: Fri, 17 Jan 2014 13:22:32 +0000 > >> If the class in skb->priority is not a leaf, apply filters from the >> selected class, not the qdisc. This lets netfilter or user space >> partially classify the packet. >> >> Signed-off-by: Harry Mason <harry.mason@smoothwall.net> > > Can I please get some reviews of this patch? > > Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] sch_htb: let skb->priority refer to non-leaf class 2014-01-17 13:22 ` [PATCH v3] " Harry Mason 2014-01-21 22:36 ` David Miller @ 2014-01-22 14:47 ` Eric Dumazet 2014-01-23 1:40 ` David Miller 2 siblings, 0 replies; 10+ messages in thread From: Eric Dumazet @ 2014-01-22 14:47 UTC (permalink / raw) To: Harry Mason; +Cc: Sergei Shtylyov, Jamal Hadi Salim, linux-netdev On Fri, 2014-01-17 at 13:22 +0000, Harry Mason wrote: > If the class in skb->priority is not a leaf, apply filters from the > selected class, not the qdisc. This lets netfilter or user space > partially classify the packet. > > Signed-off-by: Harry Mason <harry.mason@smoothwall.net> > --- Acked-by: Eric Dumazet <edumazet@google.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] sch_htb: let skb->priority refer to non-leaf class 2014-01-17 13:22 ` [PATCH v3] " Harry Mason 2014-01-21 22:36 ` David Miller 2014-01-22 14:47 ` Eric Dumazet @ 2014-01-23 1:40 ` David Miller 2 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2014-01-23 1:40 UTC (permalink / raw) To: harry.mason; +Cc: sergei.shtylyov, hadi, eric.dumazet, netdev From: Harry Mason <harry.mason@smoothwall.net> Date: Fri, 17 Jan 2014 13:22:32 +0000 > If the class in skb->priority is not a leaf, apply filters from the > selected class, not the qdisc. This lets netfilter or user space > partially classify the packet. > > Signed-off-by: Harry Mason <harry.mason@smoothwall.net> Applied, thank you. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] sch_htb: let skb->priority refer to non-leaf class 2014-01-17 10:19 ` [PATCH v2] " Harry Mason 2014-01-17 12:53 ` Sergei Shtylyov @ 2014-01-17 14:35 ` Eric Dumazet 1 sibling, 0 replies; 10+ messages in thread From: Eric Dumazet @ 2014-01-17 14:35 UTC (permalink / raw) To: Harry Mason; +Cc: Jamal Hadi Salim, linux-netdev On Fri, 2014-01-17 at 10:19 +0000, Harry Mason wrote: > If the class in skb->priority is not a leaf, apply filters from the > selected class, not the qdisc. This lets netfilter or user space > partially classify the packet. > > Signed-off-by: Harry Mason <harry.mason@smoothwall.net> > --- > > On Thu, 2014-01-16 at 08:25 -0800, Eric Dumazet wrote: > > On Thu, 2014-01-16 at 14:45 +0000, Harry Mason wrote: > > > >> + /* Start with inner filter chain if a non-leaf class is selected */ > >> + if (cl) > >> + tcf = cl->filter_list; > >> + else > >> + tcf = q->filter_list; > > > > Could this break some existing htb setups ? > > I think it is unlikely. Setting skb->priority to a non-leaf class would > be equivalent to setting it to the base qdisc. In theory an application > might rely on this if it expects the classes to be dynamic, but adding > a filter could restore the old behaviour. > Problem is : Your patch is one patch among thousands of patches, and people will install new kernels without knowing this could have an impact on their setup and might discover the problems too late (after some failure) > To me this is intuitively how it should behave, and reproduces what would > happen if a tc filter instead of netfilter had first assigned the > non-leaf class. This is definitely a patch for net-next, not net tree. > > > Also we test cl being NULL at line 222, it would be nice to not > > test it again... > > Updated below. > > net/sched/sch_htb.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c > index 717b210..8073d92 100644 > --- a/net/sched/sch_htb.c > +++ b/net/sched/sch_htb.c > @@ -219,11 +219,15 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch, > if (skb->priority == sch->handle) > return HTB_DIRECT; /* X:0 (direct flow) selected */ > cl = htb_find(skb->priority, sch); > - if (cl && cl->level == 0) > - return cl; > + if (cl) { > + if (cl->level == 0) > + return cl; > + /* Start with inner filter chain if a non-leaf class is selected */ > + tcf = cl->filter_list; > + } else > + tcf = q->filter_list; > } else { tcf = q->filter_list; } (Documentation/CodingStyle line 169) > *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; > - tcf = q->filter_list; > while (tcf && (result = tc_classify(skb, tcf, &res)) >= 0) { > #ifdef CONFIG_NET_CLS_ACT > switch (result) { ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-01-23 1:40 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-16 14:45 [PATCH] sch_htb: let skb->priority refer to non-leaf class Harry Mason 2014-01-16 16:25 ` Eric Dumazet 2014-01-17 10:19 ` [PATCH v2] " Harry Mason 2014-01-17 12:53 ` Sergei Shtylyov 2014-01-17 13:22 ` [PATCH v3] " Harry Mason 2014-01-21 22:36 ` David Miller 2014-01-22 12:41 ` Jamal Hadi Salim 2014-01-22 14:47 ` Eric Dumazet 2014-01-23 1:40 ` David Miller 2014-01-17 14:35 ` [PATCH v2] " Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).