Patchwork [net-next,v3,1/6] net_sched: remove get_stats from tc_action_ops

login
register
mail settings
Submitter WANG Cong
Date Dec. 13, 2013, 5:47 a.m.
Message ID <1386913673-8210-2-git-send-email-xiyou.wangcong@gmail.com>
Download mbox | patch
Permalink /patch/300890/
State Superseded
Delegated to: David Miller
Headers show

Comments

WANG Cong - Dec. 13, 2013, 5:47 a.m.
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(-)
David Laight - Dec. 13, 2013, 10:21 a.m.
> 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
Jamal Hadi Salim - Dec. 13, 2013, 12:05 p.m.
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
WANG Cong - Dec. 13, 2013, 6:33 p.m.
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
WANG Cong - Dec. 13, 2013, 6:38 p.m.
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
Jamal Hadi Salim - Dec. 13, 2013, 9:27 p.m.
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
Jamal Hadi Salim - Dec. 13, 2013, 9:29 p.m.
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
WANG Cong - Dec. 13, 2013, 9:34 p.m.
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
Jamal Hadi Salim - Dec. 13, 2013, 9:40 p.m.
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
WANG Cong - Dec. 13, 2013, 9:50 p.m.
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
Jamal Hadi Salim - Dec. 13, 2013, 10:19 p.m.
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
David Miller - Dec. 13, 2013, 11:21 p.m.
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
David Miller - Dec. 13, 2013, 11:23 p.m.
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
Jamal Hadi Salim - Dec. 14, 2013, 2:59 a.m.
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
David Miller - Dec. 14, 2013, 5:15 a.m.
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
Jamal Hadi Salim - Dec. 14, 2013, 12:22 p.m.
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
David Miller - Dec. 15, 2013, 3:26 a.m.
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

Patch

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 ||