| Message ID | 1386913673-8210-2-git-send-email-xiyou.wangcong@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
> From: Cong Wang > > It is not used. ... > --- a/include/net/act_api.h > +++ b/include/net/act_api.h > @@ -72,7 +72,6 @@ struct tc_action_ops { > __u32 capab; /* capabilities includes 4 bit version */ > struct module *owner; > int (*act)(struct sk_buff *, const struct tc_action *, struct tcf_result *); > - int (*get_stats)(struct sk_buff *, struct tc_action *); > int (*dump)(struct sk_buff *, struct tc_action *, int, int); > int (*cleanup)(struct tc_action *, int bind); > int (*lookup)(struct tc_action *, u32); Deleting a field out of the middle of a list of function pointers seems dangerous. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
So where is you 0/6? ;-> General: The spirit of the patches i find agreeable. I am against patch 1. Dont just remove ABI/APIs that exist. Some of these changes are substantial - did you do any testing? If you havent I can help - but at minimal i will ask you do so. I will also do more review then with whatever iteration you have around sunday. I have other patches i meant to submit - but i will do them on top of yours. cheers, jamal On 12/13/13 00:47, Cong Wang wrote: > It is not used. > > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > Cc: David S. Miller <davem@davemloft.net> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > include/net/act_api.h | 1 - > net/sched/act_api.c | 4 ---- > 2 files changed, 5 deletions(-) > > diff --git a/include/net/act_api.h b/include/net/act_api.h > index 9e90fdf..04c6825 100644 > --- a/include/net/act_api.h > +++ b/include/net/act_api.h > @@ -72,7 +72,6 @@ struct tc_action_ops { > __u32 capab; /* capabilities includes 4 bit version */ > struct module *owner; > int (*act)(struct sk_buff *, const struct tc_action *, struct tcf_result *); > - int (*get_stats)(struct sk_buff *, struct tc_action *); > int (*dump)(struct sk_buff *, struct tc_action *, int, int); > int (*cleanup)(struct tc_action *, int bind); > int (*lookup)(struct tc_action *, u32); > diff --git a/net/sched/act_api.c b/net/sched/act_api.c > index 4adbce8..51e28f7 100644 > --- a/net/sched/act_api.c > +++ b/net/sched/act_api.c > @@ -637,10 +637,6 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *a, > if (err < 0) > goto errout; > > - if (a->ops != NULL && a->ops->get_stats != NULL) > - if (a->ops->get_stats(skb, a) < 0) > - goto errout; > - > if (gnet_stats_copy_basic(&d, &h->tcf_bstats) < 0 || > gnet_stats_copy_rate_est(&d, &h->tcf_bstats, > &h->tcf_rate_est) < 0 || > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 13, 2013 at 2:21 AM, David Laight <David.Laight@aculab.com> wrote: >> From: Cong Wang >> >> It is not used. > ... >> --- a/include/net/act_api.h >> +++ b/include/net/act_api.h >> @@ -72,7 +72,6 @@ struct tc_action_ops { >> __u32 capab; /* capabilities includes 4 bit version */ >> struct module *owner; >> int (*act)(struct sk_buff *, const struct tc_action *, struct tcf_result *); >> - int (*get_stats)(struct sk_buff *, struct tc_action *); >> int (*dump)(struct sk_buff *, struct tc_action *, int, int); >> int (*cleanup)(struct tc_action *, int bind); >> int (*lookup)(struct tc_action *, u32); > > Deleting a field out of the middle of a list of function pointers > seems dangerous. Why? Keep in mind that we don't need to worry about any out-of-tree modules. :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 13, 2013 at 4:05 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > So where is you 0/6? ;-> It is there, just without any CC. :-) > General: The spirit of the patches i find agreeable. > I am against patch 1. Dont just remove ABI/APIs that exist. I don't think we care about kernel module ABI in upstream, nor we need to care about any out-of-tree module. Otherwise they are already broken by any change of sk_buff, right? > Some of these changes are substantial - did you do any testing? > If you havent I can help - but at minimal i will ask you do so. > I will also do more review then with whatever iteration you have around > sunday. I have other patches i meant to submit - but i will do them on > top of yours. > Yes, but I just have testcase for u32 filter and basic filter with mirred action. I don't have time to write testcases for other things yet. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/13/13 13:33, Cong Wang wrote: > Why? Keep in mind that we don't need to worry about any > out-of-tree modules. :) This is an API that is used to export additional stats that are not generic. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/13/13 13:38, Cong Wang wrote: > Yes, but I just have testcase for u32 filter and basic filter with > mirred action. I don't have time to write testcases for other things > yet. Good - at least you are testing. Please fix your patches based on comments you received and i will do more testing on my side when your patches are in good shape. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 13, 2013 at 1:27 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 12/13/13 13:33, Cong Wang wrote: > >> Why? Keep in mind that we don't need to worry about any >> out-of-tree modules. :) > > > This is an API that is used to export additional stats that > are not generic. > But no in-tree module implements it.... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/13/13 16:34, Cong Wang wrote:
> But no in-tree module implements it....
So? It is an API that is in there and expected by user space.
cheers,
jamal
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 13, 2013 at 1:40 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 12/13/13 16:34, Cong Wang wrote: > >> But no in-tree module implements it.... > > > So? It is an API that is in there and expected by user space. I must miss something here... Before this patch: if (a->ops != NULL && a->ops->get_stats != NULL) if (a->ops->get_stats(skb, a) < 0) goto errout; since ->get_stats is always NULL, after we remove it, no behavior is changed. So, how could this affect user-space? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/13/13 16:50, Cong Wang wrote:
> I must miss something here...
You are.
This api is for use for additional stats other than the generic
stats. It will export such extra stats to user space. There is nothing
unusual here - if you look at qdiscs and classes you will see equivalent
interfaces.
It has been there for a decade. It has been used in the past and it
may still be in use by other modules in the wild (even if i or you
dont care about them). Please dont break things.
cheers,
jamal
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: Fri, 13 Dec 2013 16:27:34 -0500 > On 12/13/13 13:33, Cong Wang wrote: > >> Why? Keep in mind that we don't need to worry about any >> out-of-tree modules. :) > > This is an API that is used to export additional stats that > are not generic. Nobody implements it. When someone does, add it back. End of discussion. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: Fri, 13 Dec 2013 17:19:23 -0500 > On 12/13/13 16:50, Cong Wang wrote: > >> I must miss something here... > > You are. > This api is for use for additional stats other than the generic > stats. It will export such extra stats to user space. There is nothing > unusual here - if you look at qdiscs and classes you will see > equivalent > interfaces. > It has been there for a decade. It has been used in the past and it > may still be in use by other modules in the wild (even if i or you > dont care about them). Please dont break things. Jamal, nobody implements this interface, therefore these extra stats are _NEVER_ provided. This is the end of the discussion, when there are in-tree users of this method you can add it back. Userland will have no idea whether it is there or not, because there is no way for it to tell one way or another. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/13/13 18:23, David Miller wrote: > Jamal, nobody implements this interface, therefore these extra > stats are _NEVER_ provided. > > This is the end of the discussion, when there are in-tree users > of this method you can add it back. Dave, I think that would be a good arguement if the feature was being provided in a new set of patches today. I dont see this as any different than removing an something from a UAPI header nobody knows any user of. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: Fri, 13 Dec 2013 21:59:01 -0500 > On 12/13/13 18:23, David Miller wrote: > >> Jamal, nobody implements this interface, therefore these extra >> stats are _NEVER_ provided. >> >> This is the end of the discussion, when there are in-tree users >> of this method you can add it back. > > Dave, I think that would be a good arguement if the feature was being > provided in a new set of patches today. > I dont see this as any different than removing an something from a > UAPI header nobody knows any user of. It is significantly different. It is an internal kernel interface that was never used. You cannot argue that, because it might have generated a netlink attribute had it been implemented, that userland has a dependency upon it. You simply can't. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/14/13 00:15, David Miller wrote: > It is significantly different. > > It is an internal kernel interface that was never used. It is not an internal API, Dave. It an external kernel module API that has been visible for many years. It provides "xstats" semantics - which is something an action user has high potential use for. It has been used in the past, just not by current in tree users. > You cannot argue that, because it might have generated a netlink > attribute had it been implemented, that userland has a dependency upon > it. The arguement is: it does expose some very strategically located attributes. "xstats" sit between basic stats and other attributes in the netlink layout (just as they do in qdisc and classes). Yes, no in tree users exist but this feature has been there for _a few years now_ and is not getting in the way of anything to deserve chopping down. Anyways, your call. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: Sat, 14 Dec 2013 07:22:26 -0500 > On 12/14/13 00:15, David Miller wrote: > >> It is significantly different. >> >> It is an internal kernel interface that was never used. > > It is not an internal API, Dave. > It an external kernel module API that has been visible for many years. There are no stable kernel APIs, I'm sorry if this is news to you. Documentation/stable_api_nonsense.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/act_api.h b/include/net/act_api.h index 9e90fdf..04c6825 100644 --- a/include/net/act_api.h +++ b/include/net/act_api.h @@ -72,7 +72,6 @@ struct tc_action_ops { __u32 capab; /* capabilities includes 4 bit version */ struct module *owner; int (*act)(struct sk_buff *, const struct tc_action *, struct tcf_result *); - int (*get_stats)(struct sk_buff *, struct tc_action *); int (*dump)(struct sk_buff *, struct tc_action *, int, int); int (*cleanup)(struct tc_action *, int bind); int (*lookup)(struct tc_action *, u32); diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 4adbce8..51e28f7 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -637,10 +637,6 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *a, if (err < 0) goto errout; - if (a->ops != NULL && a->ops->get_stats != NULL) - if (a->ops->get_stats(skb, a) < 0) - goto errout; - if (gnet_stats_copy_basic(&d, &h->tcf_bstats) < 0 || gnet_stats_copy_rate_est(&d, &h->tcf_bstats, &h->tcf_rate_est) < 0 ||
It is not used. Cc: Jamal Hadi Salim <jhs@mojatatu.com> Cc: David S. Miller <davem@davemloft.net> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- include/net/act_api.h | 1 - net/sched/act_api.c | 4 ---- 2 files changed, 5 deletions(-)