| Message ID | 20160226155349.5338.74615.stgit@john-Precision-Tower-5810 |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Fri, Feb 26, 2016 at 04:53:49PM CET, john.fastabend@gmail.com wrote: >The offload decision was originally very basic and tied to if the dev >implemented the appropriate ndo op hook. The next step is to allow >the user to more flexibly define if any paticular rule should be >offloaded or not. In order to have this logic in one function lift >the current check into a helper routine tc_should_offload(). > >Signed-off-by: John Fastabend <john.r.fastabend@intel.com> Acked-by: Jiri Pirko <jiri@mellanox.com>
On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend <john.fastabend@gmail.com> wrote: > The offload decision was originally very basic and tied to if the dev > implemented the appropriate ndo op hook. The next step is to allow > the user to more flexibly define if any paticular rule should be > offloaded or not. In order to have this logic in one function lift > the current check into a helper routine tc_should_offload(). > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > include/net/pkt_cls.h | 5 +++++ > net/sched/cls_u32.c | 8 ++++---- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h > index 2121df5..e64d20b 100644 > --- a/include/net/pkt_cls.h > +++ b/include/net/pkt_cls.h > @@ -392,4 +392,9 @@ struct tc_cls_u32_offload { > }; > }; > > +static inline bool tc_should_offload(struct net_device *dev) > +{ > + return dev->netdev_ops->ndo_setup_tc; > +} > + These should be protected by CONFIG_NET_CLS_U32, no?
On 16-02-26 09:39 AM, Cong Wang wrote: > On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend > <john.fastabend@gmail.com> wrote: >> The offload decision was originally very basic and tied to if the dev >> implemented the appropriate ndo op hook. The next step is to allow >> the user to more flexibly define if any paticular rule should be >> offloaded or not. In order to have this logic in one function lift >> the current check into a helper routine tc_should_offload(). >> >> Signed-off-by: John Fastabend <john.r.fastabend@intel.com> >> --- >> include/net/pkt_cls.h | 5 +++++ >> net/sched/cls_u32.c | 8 ++++---- >> 2 files changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >> index 2121df5..e64d20b 100644 >> --- a/include/net/pkt_cls.h >> +++ b/include/net/pkt_cls.h >> @@ -392,4 +392,9 @@ struct tc_cls_u32_offload { >> }; >> }; >> >> +static inline bool tc_should_offload(struct net_device *dev) >> +{ >> + return dev->netdev_ops->ndo_setup_tc; >> +} >> + > > These should be protected by CONFIG_NET_CLS_U32, no? > Its not necessary it is a completely general function and I only lifted it out of cls_u32 so that the cls_flower classifier could also use it. I don't see the need off-hand to have it wrapped in an ORd ifdef statement where its (CONFIG_NET_CLS_U32 | CONFIG_NET_CLS_X ...). Any particular reason you were thnking it should be wrapped in ifdefs? Thanks for taking a look at the patches. .John
On Fri, Feb 26, 2016 at 8:24 PM, John Fastabend <john.fastabend@gmail.com> wrote: > On 16-02-26 09:39 AM, Cong Wang wrote: >> On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend >> <john.fastabend@gmail.com> wrote: >>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >>> index 2121df5..e64d20b 100644 >>> --- a/include/net/pkt_cls.h >>> +++ b/include/net/pkt_cls.h >>> @@ -392,4 +392,9 @@ struct tc_cls_u32_offload { >>> }; >>> }; >>> >>> +static inline bool tc_should_offload(struct net_device *dev) >>> +{ >>> + return dev->netdev_ops->ndo_setup_tc; >>> +} >>> + >> >> These should be protected by CONFIG_NET_CLS_U32, no? >> > > Its not necessary it is a completely general function and I only > lifted it out of cls_u32 so that the cls_flower classifier could > also use it. > > I don't see the need off-hand to have it wrapped in an ORd ifdef > statement where its (CONFIG_NET_CLS_U32 | CONFIG_NET_CLS_X ...). > Any particular reason you were thnking it should be wrapped in ifdefs? > Not a big deal. I just feel these don't need to compile when I have CONFIG_NET_CLS_U32=n. Thanks.
On 16-02-27 08:28 PM, Cong Wang wrote: > On Fri, Feb 26, 2016 at 8:24 PM, John Fastabend > <john.fastabend@gmail.com> wrote: >> On 16-02-26 09:39 AM, Cong Wang wrote: >>> On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend >>> <john.fastabend@gmail.com> wrote: >>>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >>>> index 2121df5..e64d20b 100644 >>>> --- a/include/net/pkt_cls.h >>>> +++ b/include/net/pkt_cls.h >>>> @@ -392,4 +392,9 @@ struct tc_cls_u32_offload { >>>> }; >>>> }; >>>> >>>> +static inline bool tc_should_offload(struct net_device *dev) >>>> +{ >>>> + return dev->netdev_ops->ndo_setup_tc; >>>> +} >>>> + >>> >>> These should be protected by CONFIG_NET_CLS_U32, no? >>> >> >> Its not necessary it is a completely general function and I only >> lifted it out of cls_u32 so that the cls_flower classifier could >> also use it. >> >> I don't see the need off-hand to have it wrapped in an ORd ifdef >> statement where its (CONFIG_NET_CLS_U32 | CONFIG_NET_CLS_X ...). >> Any particular reason you were thnking it should be wrapped in ifdefs? >> > > Not a big deal. > > I just feel these don't need to compile when I have CONFIG_NET_CLS_U32=n. > > Thanks. > Well because this is 'static inline' gcc should just remove it if it is not used. Assuming non-ancient gcc and normal compile flags, e.g. you are not including -fkeep-inline-functions or something. So just to keep it readable I would prefer to just leave it as is. Thanks, John
Mon, Feb 29, 2016 at 07:40:53PM CET, john.fastabend@gmail.com wrote: >On 16-02-27 08:28 PM, Cong Wang wrote: >> On Fri, Feb 26, 2016 at 8:24 PM, John Fastabend >> <john.fastabend@gmail.com> wrote: >>> On 16-02-26 09:39 AM, Cong Wang wrote: >>>> On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend >>>> <john.fastabend@gmail.com> wrote: >>>>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >>>>> index 2121df5..e64d20b 100644 >>>>> --- a/include/net/pkt_cls.h >>>>> +++ b/include/net/pkt_cls.h >>>>> @@ -392,4 +392,9 @@ struct tc_cls_u32_offload { >>>>> }; >>>>> }; >>>>> >>>>> +static inline bool tc_should_offload(struct net_device *dev) >>>>> +{ >>>>> + return dev->netdev_ops->ndo_setup_tc; >>>>> +} >>>>> + >>>> >>>> These should be protected by CONFIG_NET_CLS_U32, no? >>>> >>> >>> Its not necessary it is a completely general function and I only >>> lifted it out of cls_u32 so that the cls_flower classifier could >>> also use it. >>> >>> I don't see the need off-hand to have it wrapped in an ORd ifdef >>> statement where its (CONFIG_NET_CLS_U32 | CONFIG_NET_CLS_X ...). >>> Any particular reason you were thnking it should be wrapped in ifdefs? >>> >> >> Not a big deal. >> >> I just feel these don't need to compile when I have CONFIG_NET_CLS_U32=n. >> >> Thanks. >> > >Well because this is 'static inline' gcc should just remove it >if it is not used. Assuming non-ancient gcc and normal compile >flags, e.g. you are not including -fkeep-inline-functions or >something. > >So just to keep it readable I would prefer to just leave it >as is. Definitelly. cls_flower will use it in very near future. Making it dependent on CONFIG_NET_CLS_U32 makes 0 sense to me.
On Mon, Feb 29, 2016 at 10:58 AM, Jiri Pirko <jiri@resnulli.us> wrote: > Mon, Feb 29, 2016 at 07:40:53PM CET, john.fastabend@gmail.com wrote: >>On 16-02-27 08:28 PM, Cong Wang wrote: >>> On Fri, Feb 26, 2016 at 8:24 PM, John Fastabend >>> <john.fastabend@gmail.com> wrote: >>>> On 16-02-26 09:39 AM, Cong Wang wrote: >>>>> On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend >>>>> <john.fastabend@gmail.com> wrote: >>>>>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >>>>>> index 2121df5..e64d20b 100644 >>>>>> --- a/include/net/pkt_cls.h >>>>>> +++ b/include/net/pkt_cls.h >>>>>> @@ -392,4 +392,9 @@ struct tc_cls_u32_offload { >>>>>> }; >>>>>> }; >>>>>> >>>>>> +static inline bool tc_should_offload(struct net_device *dev) >>>>>> +{ >>>>>> + return dev->netdev_ops->ndo_setup_tc; >>>>>> +} >>>>>> + >>>>> >>>>> These should be protected by CONFIG_NET_CLS_U32, no? >>>>> >>>> >>>> Its not necessary it is a completely general function and I only >>>> lifted it out of cls_u32 so that the cls_flower classifier could >>>> also use it. >>>> >>>> I don't see the need off-hand to have it wrapped in an ORd ifdef >>>> statement where its (CONFIG_NET_CLS_U32 | CONFIG_NET_CLS_X ...). >>>> Any particular reason you were thnking it should be wrapped in ifdefs? >>>> >>> >>> Not a big deal. >>> >>> I just feel these don't need to compile when I have CONFIG_NET_CLS_U32=n. >>> >>> Thanks. >>> >> >>Well because this is 'static inline' gcc should just remove it >>if it is not used. Assuming non-ancient gcc and normal compile >>flags, e.g. you are not including -fkeep-inline-functions or >>something. >> >>So just to keep it readable I would prefer to just leave it >>as is. > > Definitelly. cls_flower will use it in very near future. Making it > dependent on CONFIG_NET_CLS_U32 makes 0 sense to me. Oh, why then do you have u32 in the struct name tc_cls_u32_offload? (Note that in the above I said "these" not "this", so I never only refer to tc_should_offload)
On 16-02-29 01:25 PM, Cong Wang wrote: > On Mon, Feb 29, 2016 at 10:58 AM, Jiri Pirko <jiri@resnulli.us> wrote: >> Mon, Feb 29, 2016 at 07:40:53PM CET, john.fastabend@gmail.com wrote: >>> On 16-02-27 08:28 PM, Cong Wang wrote: >>>> On Fri, Feb 26, 2016 at 8:24 PM, John Fastabend >>>> <john.fastabend@gmail.com> wrote: >>>>> On 16-02-26 09:39 AM, Cong Wang wrote: >>>>>> On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend >>>>>> <john.fastabend@gmail.com> wrote: >>>>>>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >>>>>>> index 2121df5..e64d20b 100644 >>>>>>> --- a/include/net/pkt_cls.h >>>>>>> +++ b/include/net/pkt_cls.h >>>>>>> @@ -392,4 +392,9 @@ struct tc_cls_u32_offload { >>>>>>> }; >>>>>>> }; >>>>>>> >>>>>>> +static inline bool tc_should_offload(struct net_device *dev) >>>>>>> +{ >>>>>>> + return dev->netdev_ops->ndo_setup_tc; >>>>>>> +} >>>>>>> + >>>>>> >>>>>> These should be protected by CONFIG_NET_CLS_U32, no? >>>>>> >>>>> >>>>> Its not necessary it is a completely general function and I only >>>>> lifted it out of cls_u32 so that the cls_flower classifier could >>>>> also use it. >>>>> >>>>> I don't see the need off-hand to have it wrapped in an ORd ifdef >>>>> statement where its (CONFIG_NET_CLS_U32 | CONFIG_NET_CLS_X ...). >>>>> Any particular reason you were thnking it should be wrapped in ifdefs? >>>>> >>>> >>>> Not a big deal. >>>> >>>> I just feel these don't need to compile when I have CONFIG_NET_CLS_U32=n. >>>> >>>> Thanks. >>>> >>> >>> Well because this is 'static inline' gcc should just remove it >>> if it is not used. Assuming non-ancient gcc and normal compile >>> flags, e.g. you are not including -fkeep-inline-functions or >>> something. >>> >>> So just to keep it readable I would prefer to just leave it >>> as is. >> >> Definitelly. cls_flower will use it in very near future. Making it >> dependent on CONFIG_NET_CLS_U32 makes 0 sense to me. > > Oh, why then do you have u32 in the struct name tc_cls_u32_offload? > > (Note that in the above I said "these" not "this", so I never only refer > to tc_should_offload) > hmm yeah that likely wont be needed by flower although it could be used. I still think its best to leave this as is there doesn't seem to be a very strong precedent to wrap any of the other structs/fields/etc in pkt_cls.h into their respective ifdef/endif blocks. And I think it starts to get a bit much if we do. I'm trusting gcc here can do the right thing when these are included but never used. Thanks, John
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index 2121df5..e64d20b 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -392,4 +392,9 @@ struct tc_cls_u32_offload { }; }; +static inline bool tc_should_offload(struct net_device *dev) +{ + return dev->netdev_ops->ndo_setup_tc; +} + #endif diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c index d54bc94..24e888b 100644 --- a/net/sched/cls_u32.c +++ b/net/sched/cls_u32.c @@ -434,7 +434,7 @@ static void u32_remove_hw_knode(struct tcf_proto *tp, u32 handle) offload.type = TC_SETUP_CLSU32; offload.cls_u32 = &u32_offload; - if (dev->netdev_ops->ndo_setup_tc) { + if (tc_should_offload(dev)) { offload.cls_u32->command = TC_CLSU32_DELETE_KNODE; offload.cls_u32->knode.handle = handle; dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle, @@ -451,7 +451,7 @@ static void u32_replace_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h) offload.type = TC_SETUP_CLSU32; offload.cls_u32 = &u32_offload; - if (dev->netdev_ops->ndo_setup_tc) { + if (tc_should_offload(dev)) { offload.cls_u32->command = TC_CLSU32_NEW_HNODE; offload.cls_u32->hnode.divisor = h->divisor; offload.cls_u32->hnode.handle = h->handle; @@ -471,7 +471,7 @@ static void u32_clear_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h) offload.type = TC_SETUP_CLSU32; offload.cls_u32 = &u32_offload; - if (dev->netdev_ops->ndo_setup_tc) { + if (tc_should_offload(dev)) { offload.cls_u32->command = TC_CLSU32_DELETE_HNODE; offload.cls_u32->hnode.divisor = h->divisor; offload.cls_u32->hnode.handle = h->handle; @@ -491,7 +491,7 @@ static void u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n) offload.type = TC_SETUP_CLSU32; offload.cls_u32 = &u32_offload; - if (dev->netdev_ops->ndo_setup_tc) { + if (tc_should_offload(dev)) { offload.cls_u32->command = TC_CLSU32_REPLACE_KNODE; offload.cls_u32->knode.handle = n->handle; offload.cls_u32->knode.fshift = n->fshift;
The offload decision was originally very basic and tied to if the dev implemented the appropriate ndo op hook. The next step is to allow the user to more flexibly define if any paticular rule should be offloaded or not. In order to have this logic in one function lift the current check into a helper routine tc_should_offload(). Signed-off-by: John Fastabend <john.r.fastabend@intel.com> --- include/net/pkt_cls.h | 5 +++++ net/sched/cls_u32.c | 8 ++++---- 2 files changed, 9 insertions(+), 4 deletions(-)