[PATCH net-next v5] net: psample: Introduce stubs to remove NIC driver dependency

[PATCH net-next v5] net: psample: Introduce stubs to remove NIC driver dependency

[Chris Mi]

In order to send sampled packets to userspace, NIC driver calls
psample api directly. But it creates a hard dependency on module
psample. Introduce psample_ops to remove the hard dependency.
It is initialized when psample module is loaded and set to NULL
when the module is unloaded.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Chris Mi <cmi@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
 - fix sparse errors
v2->v3:
 - remove inline
v3->v4:
 - add inline back
v4->v5:
 - address Jakub's comments

 include/net/psample.h    | 26 ++++++++++++++++++++++++++
 net/psample/psample.c    | 14 +++++++++++++-
 net/sched/Makefile       |  2 +-
 net/sched/psample_stub.c |  5 +++++
 4 files changed, 45 insertions(+), 2 deletions(-)
 create mode 100644 net/sched/psample_stub.c

diff --git a/include/net/psample.h b/include/net/psample.h
index 68ae16bb0a4a..d0f1cfc56f6f 100644
--- a/include/net/psample.h
+++ b/include/net/psample.h
@@ -14,6 +14,15 @@ struct psample_group {
 	struct rcu_head rcu;
 };
 
+struct psample_ops {
+	void (*sample_packet)(struct psample_group *group, struct sk_buff *skb,
+			      u32 trunc_size, int in_ifindex, int out_ifindex,
+			      u32 sample_rate);
+
+};
+
+extern const struct psample_ops __rcu *psample_ops __read_mostly;
+
 struct psample_group *psample_group_get(struct net *net, u32 group_num);
 void psample_group_take(struct psample_group *group);
 void psample_group_put(struct psample_group *group);
@@ -35,4 +44,21 @@ static inline void psample_sample_packet(struct psample_group *group,
 
 #endif
 
+static inline void
+psample_nic_sample_packet(struct psample_group *group,
+			  struct sk_buff *skb, u32 trunc_size,
+			  int in_ifindex, int out_ifindex,
+			  u32 sample_rate)
+{
+	const struct psample_ops *ops;
+
+	rcu_read_lock();
+	ops = rcu_dereference(psample_ops);
+	if (ops)
+		ops->sample_packet(group, skb, trunc_size,
+				   in_ifindex, out_ifindex,
+				   sample_rate);
+	rcu_read_unlock();
+}
+
 #endif /* __NET_PSAMPLE_H */
diff --git a/net/psample/psample.c b/net/psample/psample.c
index 33e238c965bd..983ca5b698fe 100644
--- a/net/psample/psample.c
+++ b/net/psample/psample.c
@@ -8,6 +8,7 @@
 #include <linux/kernel.h>
 #include <linux/skbuff.h>
 #include <linux/module.h>
+#include <linux/rcupdate.h>
 #include <net/net_namespace.h>
 #include <net/sock.h>
 #include <net/netlink.h>
@@ -35,6 +36,10 @@ static const struct genl_multicast_group psample_nl_mcgrps[] = {
 
 static struct genl_family psample_nl_family __ro_after_init;
 
+static const struct psample_ops psample_sample_ops = {
+	.sample_packet	= psample_sample_packet,
+};
+
 static int psample_group_nl_fill(struct sk_buff *msg,
 				 struct psample_group *group,
 				 enum psample_command cmd, u32 portid, u32 seq,
@@ -456,11 +461,18 @@ EXPORT_SYMBOL_GPL(psample_sample_packet);
 
 static int __init psample_module_init(void)
 {
-	return genl_register_family(&psample_nl_family);
+	int ret;
+
+	ret = genl_register_family(&psample_nl_family);
+	if (!ret)
+		RCU_INIT_POINTER(psample_ops, &psample_sample_ops);
+	return ret;
 }
 
 static void __exit psample_module_exit(void)
 {
+	rcu_assign_pointer(psample_ops, NULL);
+	synchronize_rcu();
 	genl_unregister_family(&psample_nl_family);
 }
 
diff --git a/net/sched/Makefile b/net/sched/Makefile
index dd14ef413fda..0d92bb98bb26 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -3,7 +3,7 @@
 # Makefile for the Linux Traffic Control Unit.
 #
 
-obj-y	:= sch_generic.o sch_mq.o
+obj-y	:= sch_generic.o sch_mq.o psample_stub.o
 
 obj-$(CONFIG_INET)		+= sch_frag.o
 obj-$(CONFIG_NET_SCHED)		+= sch_api.o sch_blackhole.o
diff --git a/net/sched/psample_stub.c b/net/sched/psample_stub.c
new file mode 100644
index 000000000000..0541b8c5100d
--- /dev/null
+++ b/net/sched/psample_stub.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+/* Copyright (c) 2021 Mellanox Technologies. */
+
+const struct psample_ops __rcu *psample_ops __read_mostly;
+EXPORT_SYMBOL_GPL(psample_ops);
-- 
2.26.2

Re: [PATCH net-next v5] net: psample: Introduce stubs to remove NIC driver dependency

[Jakub Kicinski]

On Sat, 30 Jan 2021 10:33:19 +0800 Chris Mi wrote:
> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> +/* Copyright (c) 2021 Mellanox Technologies. */
> +
> +const struct psample_ops __rcu *psample_ops __read_mostly;
> +EXPORT_SYMBOL_GPL(psample_ops);

Please explain to me how you could possibly have compile tested this
and not caught that it doesn't build.

Re: [PATCH net-next v5] net: psample: Introduce stubs to remove NIC driver dependency

[Ido Schimmel]

On Sat, Jan 30, 2021 at 10:33:19AM +0800, Chris Mi wrote:
> In order to send sampled packets to userspace, NIC driver calls
> psample api directly. But it creates a hard dependency on module
> psample. Introduce psample_ops to remove the hard dependency.
> It is initialized when psample module is loaded and set to NULL
> when the module is unloaded.
> 
> Reported-by: kernel test robot <lkp@intel.com>

This belongs in the changelog (that should be part of the commit
message). Something like "Fix xxx reported by kernel test robot".

> Signed-off-by: Chris Mi <cmi@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> ---
> v1->v2:
>  - fix sparse errors
> v2->v3:
>  - remove inline
> v3->v4:
>  - add inline back
> v4->v5:
>  - address Jakub's comments
> 
>  include/net/psample.h    | 26 ++++++++++++++++++++++++++
>  net/psample/psample.c    | 14 +++++++++++++-
>  net/sched/Makefile       |  2 +-
>  net/sched/psample_stub.c |  5 +++++
>  4 files changed, 45 insertions(+), 2 deletions(-)
>  create mode 100644 net/sched/psample_stub.c
> 
> diff --git a/include/net/psample.h b/include/net/psample.h
> index 68ae16bb0a4a..d0f1cfc56f6f 100644
> --- a/include/net/psample.h
> +++ b/include/net/psample.h
> @@ -14,6 +14,15 @@ struct psample_group {
>  	struct rcu_head rcu;
>  };
>  
> +struct psample_ops {
> +	void (*sample_packet)(struct psample_group *group, struct sk_buff *skb,
> +			      u32 trunc_size, int in_ifindex, int out_ifindex,
> +			      u32 sample_rate);
> +
> +};
> +
> +extern const struct psample_ops __rcu *psample_ops __read_mostly;
> +
>  struct psample_group *psample_group_get(struct net *net, u32 group_num);
>  void psample_group_take(struct psample_group *group);
>  void psample_group_put(struct psample_group *group);
> @@ -35,4 +44,21 @@ static inline void psample_sample_packet(struct psample_group *group,
>  
>  #endif
>  
> +static inline void
> +psample_nic_sample_packet(struct psample_group *group,
> +			  struct sk_buff *skb, u32 trunc_size,
> +			  int in_ifindex, int out_ifindex,
> +			  u32 sample_rate)
> +{
> +	const struct psample_ops *ops;
> +
> +	rcu_read_lock();
> +	ops = rcu_dereference(psample_ops);
> +	if (ops)
> +		ops->sample_packet(group, skb, trunc_size,
> +				   in_ifindex, out_ifindex,
> +				   sample_rate);
> +	rcu_read_unlock();
> +}
> +
>  #endif /* __NET_PSAMPLE_H */
> diff --git a/net/psample/psample.c b/net/psample/psample.c
> index 33e238c965bd..983ca5b698fe 100644
> --- a/net/psample/psample.c
> +++ b/net/psample/psample.c
> @@ -8,6 +8,7 @@
>  #include <linux/kernel.h>
>  #include <linux/skbuff.h>
>  #include <linux/module.h>
> +#include <linux/rcupdate.h>
>  #include <net/net_namespace.h>
>  #include <net/sock.h>
>  #include <net/netlink.h>
> @@ -35,6 +36,10 @@ static const struct genl_multicast_group psample_nl_mcgrps[] = {
>  
>  static struct genl_family psample_nl_family __ro_after_init;
>  
> +static const struct psample_ops psample_sample_ops = {
> +	.sample_packet	= psample_sample_packet,
> +};
> +
>  static int psample_group_nl_fill(struct sk_buff *msg,
>  				 struct psample_group *group,
>  				 enum psample_command cmd, u32 portid, u32 seq,
> @@ -456,11 +461,18 @@ EXPORT_SYMBOL_GPL(psample_sample_packet);
>  
>  static int __init psample_module_init(void)
>  {
> -	return genl_register_family(&psample_nl_family);
> +	int ret;
> +
> +	ret = genl_register_family(&psample_nl_family);
> +	if (!ret)
> +		RCU_INIT_POINTER(psample_ops, &psample_sample_ops);
> +	return ret;
>  }
>  
>  static void __exit psample_module_exit(void)
>  {
> +	rcu_assign_pointer(psample_ops, NULL);
> +	synchronize_rcu();
>  	genl_unregister_family(&psample_nl_family);
>  }
>  
> diff --git a/net/sched/Makefile b/net/sched/Makefile
> index dd14ef413fda..0d92bb98bb26 100644
> --- a/net/sched/Makefile
> +++ b/net/sched/Makefile
> @@ -3,7 +3,7 @@
>  # Makefile for the Linux Traffic Control Unit.
>  #
>  
> -obj-y	:= sch_generic.o sch_mq.o
> +obj-y	:= sch_generic.o sch_mq.o psample_stub.o

Why the stub is under net/sched/ when psample is at net/psample/?
psample is not the same as 'act_sample'.

>  
>  obj-$(CONFIG_INET)		+= sch_frag.o
>  obj-$(CONFIG_NET_SCHED)		+= sch_api.o sch_blackhole.o
> diff --git a/net/sched/psample_stub.c b/net/sched/psample_stub.c
> new file mode 100644
> index 000000000000..0541b8c5100d
> --- /dev/null
> +++ b/net/sched/psample_stub.c
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB

psample has "// SPDX-License-Identifier: GPL-2.0-only"

> +/* Copyright (c) 2021 Mellanox Technologies. */
> +
> +const struct psample_ops __rcu *psample_ops __read_mostly;
> +EXPORT_SYMBOL_GPL(psample_ops);
> -- 
> 2.26.2
> 

Re: [PATCH net-next v5] net: psample: Introduce stubs to remove NIC driver dependency

[Chris Mi]

On 1/30/2021 2:59 PM, Jakub Kicinski wrote:
> On Sat, 30 Jan 2021 10:33:19 +0800 Chris Mi wrote:
>> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>> +/* Copyright (c) 2021 Mellanox Technologies. */
>> +
>> +const struct psample_ops __rcu *psample_ops __read_mostly;
>> +EXPORT_SYMBOL_GPL(psample_ops);
> Please explain to me how you could possibly have compile tested this
> and not caught that it doesn't build.
Sorry, I don't understand which issue you are talking about. Do you mean
the issue sparse found before or new issues in v5?

Thanks.

Re: [PATCH net-next v5] net: psample: Introduce stubs to remove NIC driver dependency

[Chris Mi]

On 1/30/2021 10:52 PM, Ido Schimmel wrote:
> On Sat, Jan 30, 2021 at 10:33:19AM +0800, Chris Mi wrote:
>> In order to send sampled packets to userspace, NIC driver calls
>> psample api directly. But it creates a hard dependency on module
>> psample. Introduce psample_ops to remove the hard dependency.
>> It is initialized when psample module is loaded and set to NULL
>> when the module is unloaded.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
> This belongs in the changelog (that should be part of the commit
> message). Something like "Fix xxx reported by kernel test robot".
But I see existing commits have it. And Jakub didn't ask me to do it. So 
I think
I'd better keep it.
>
>> Signed-off-by: Chris Mi <cmi@nvidia.com>
>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>> v1->v2:
>>   - fix sparse errors
>> v2->v3:
>>   - remove inline
>> v3->v4:
>>   - add inline back
>> v4->v5:
>>   - address Jakub's comments
>>
>>   include/net/psample.h    | 26 ++++++++++++++++++++++++++
>>   net/psample/psample.c    | 14 +++++++++++++-
>>   net/sched/Makefile       |  2 +-
>>   net/sched/psample_stub.c |  5 +++++
>>   4 files changed, 45 insertions(+), 2 deletions(-)
>>   create mode 100644 net/sched/psample_stub.c
>>
>> diff --git a/include/net/psample.h b/include/net/psample.h
>> index 68ae16bb0a4a..d0f1cfc56f6f 100644
>> --- a/include/net/psample.h
>> +++ b/include/net/psample.h
>> @@ -14,6 +14,15 @@ struct psample_group {
>>   	struct rcu_head rcu;
>>   };
>>   
>> +struct psample_ops {
>> +	void (*sample_packet)(struct psample_group *group, struct sk_buff *skb,
>> +			      u32 trunc_size, int in_ifindex, int out_ifindex,
>> +			      u32 sample_rate);
>> +
>> +};
>> +
>> +extern const struct psample_ops __rcu *psample_ops __read_mostly;
>> +
>>   struct psample_group *psample_group_get(struct net *net, u32 group_num);
>>   void psample_group_take(struct psample_group *group);
>>   void psample_group_put(struct psample_group *group);
>> @@ -35,4 +44,21 @@ static inline void psample_sample_packet(struct psample_group *group,
>>   
>>   #endif
>>   
>> +static inline void
>> +psample_nic_sample_packet(struct psample_group *group,
>> +			  struct sk_buff *skb, u32 trunc_size,
>> +			  int in_ifindex, int out_ifindex,
>> +			  u32 sample_rate)
>> +{
>> +	const struct psample_ops *ops;
>> +
>> +	rcu_read_lock();
>> +	ops = rcu_dereference(psample_ops);
>> +	if (ops)
>> +		ops->sample_packet(group, skb, trunc_size,
>> +				   in_ifindex, out_ifindex,
>> +				   sample_rate);
>> +	rcu_read_unlock();
>> +}
>> +
>>   #endif /* __NET_PSAMPLE_H */
>> diff --git a/net/psample/psample.c b/net/psample/psample.c
>> index 33e238c965bd..983ca5b698fe 100644
>> --- a/net/psample/psample.c
>> +++ b/net/psample/psample.c
>> @@ -8,6 +8,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/skbuff.h>
>>   #include <linux/module.h>
>> +#include <linux/rcupdate.h>
>>   #include <net/net_namespace.h>
>>   #include <net/sock.h>
>>   #include <net/netlink.h>
>> @@ -35,6 +36,10 @@ static const struct genl_multicast_group psample_nl_mcgrps[] = {
>>   
>>   static struct genl_family psample_nl_family __ro_after_init;
>>   
>> +static const struct psample_ops psample_sample_ops = {
>> +	.sample_packet	= psample_sample_packet,
>> +};
>> +
>>   static int psample_group_nl_fill(struct sk_buff *msg,
>>   				 struct psample_group *group,
>>   				 enum psample_command cmd, u32 portid, u32 seq,
>> @@ -456,11 +461,18 @@ EXPORT_SYMBOL_GPL(psample_sample_packet);
>>   
>>   static int __init psample_module_init(void)
>>   {
>> -	return genl_register_family(&psample_nl_family);
>> +	int ret;
>> +
>> +	ret = genl_register_family(&psample_nl_family);
>> +	if (!ret)
>> +		RCU_INIT_POINTER(psample_ops, &psample_sample_ops);
>> +	return ret;
>>   }
>>   
>>   static void __exit psample_module_exit(void)
>>   {
>> +	rcu_assign_pointer(psample_ops, NULL);
>> +	synchronize_rcu();
>>   	genl_unregister_family(&psample_nl_family);
>>   }
>>   
>> diff --git a/net/sched/Makefile b/net/sched/Makefile
>> index dd14ef413fda..0d92bb98bb26 100644
>> --- a/net/sched/Makefile
>> +++ b/net/sched/Makefile
>> @@ -3,7 +3,7 @@
>>   # Makefile for the Linux Traffic Control Unit.
>>   #
>>   
>> -obj-y	:= sch_generic.o sch_mq.o
>> +obj-y	:= sch_generic.o sch_mq.o psample_stub.o
> Why the stub is under net/sched/ when psample is at net/psample/?
> psample is not the same as 'act_sample'.
If this stub is in net/psample and compiled to psample kernel module, NIC
driver still depends on it. It must be in kernel. And I think net/sched 
is the
only place we can put. If we put it in somewhere else, like net/, I 
guess we'll
get more objections.
>
>>   
>>   obj-$(CONFIG_INET)		+= sch_frag.o
>>   obj-$(CONFIG_NET_SCHED)		+= sch_api.o sch_blackhole.o
>> diff --git a/net/sched/psample_stub.c b/net/sched/psample_stub.c
>> new file mode 100644
>> index 000000000000..0541b8c5100d
>> --- /dev/null
>> +++ b/net/sched/psample_stub.c
>> @@ -0,0 +1,5 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> psample has "// SPDX-License-Identifier: GPL-2.0-only"
Done. Thanks.
>
>> +/* Copyright (c) 2021 Mellanox Technologies. */
>> +
>> +const struct psample_ops __rcu *psample_ops __read_mostly;
>> +EXPORT_SYMBOL_GPL(psample_ops);
>> -- 
>> 2.26.2
>>

Re: [PATCH net-next v5] net: psample: Introduce stubs to remove NIC driver dependency

[Ido Schimmel]

On Mon, Feb 01, 2021 at 10:00:48AM +0800, Chris Mi wrote:
> On 1/30/2021 10:52 PM, Ido Schimmel wrote:
> > On Sat, Jan 30, 2021 at 10:33:19AM +0800, Chris Mi wrote:
> > > In order to send sampled packets to userspace, NIC driver calls
> > > psample api directly. But it creates a hard dependency on module
> > > psample. Introduce psample_ops to remove the hard dependency.
> > > It is initialized when psample module is loaded and set to NULL
> > > when the module is unloaded.
> > > 
> > > Reported-by: kernel test robot <lkp@intel.com>
> > This belongs in the changelog (that should be part of the commit
> > message). Something like "Fix xxx reported by kernel test robot".
> But I see existing commits have it.

It is used by commits whose sole purpose is to fix an issue that was
reported by the kernel test robot. In this case the robot merely
reported an issue with your v1. If you want to give credit, use the form
I suggested above. It is misleading otherwise.

Re: [PATCH net-next v5] net: psample: Introduce stubs to remove NIC driver dependency

[Jakub Kicinski]

On Mon, 1 Feb 2021 09:32:15 +0800 Chris Mi wrote:
> On 1/30/2021 2:59 PM, Jakub Kicinski wrote:
> > On Sat, 30 Jan 2021 10:33:19 +0800 Chris Mi wrote:  
> >> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> >> +/* Copyright (c) 2021 Mellanox Technologies. */
> >> +
> >> +const struct psample_ops __rcu *psample_ops __read_mostly;
> >> +EXPORT_SYMBOL_GPL(psample_ops);  
> > Please explain to me how you could possibly have compile tested this
> > and not caught that it doesn't build.  
> Sorry, I don't understand which issue you are talking about. Do you mean
> the issue sparse found before or new issues in v5?

There is no include here now, and it uses EXPORT_SYMBOL_GPL() 
and a bunch of decorators.

Re: [PATCH net-next v5] net: psample: Introduce stubs to remove NIC driver dependency

[Jakub Kicinski]

On Mon, 1 Feb 2021 20:06:06 +0200 Ido Schimmel wrote:
> On Mon, Feb 01, 2021 at 10:00:48AM +0800, Chris Mi wrote:
> > On 1/30/2021 10:52 PM, Ido Schimmel wrote:  
> > > This belongs in the changelog (that should be part of the commit
> > > message). Something like "Fix xxx reported by kernel test robot".  
> > But I see existing commits have it.  
> 
> It is used by commits whose sole purpose is to fix an issue that was
> reported by the kernel test robot. In this case the robot merely
> reported an issue with your v1. If you want to give credit, use the form
> I suggested above. It is misleading otherwise.

Personally I fully agree. But some people pushed back on this in 
the past saying buildbot should be credited for early detection, 
too - otherwise the funding may dry up. So I don't care either 
way now :)

Re: [PATCH net-next v5] net: psample: Introduce stubs to remove NIC driver dependency

[Chris Mi]

On 2/2/2021 5:10 AM, Jakub Kicinski wrote:
> On Mon, 1 Feb 2021 09:32:15 +0800 Chris Mi wrote:
>> On 1/30/2021 2:59 PM, Jakub Kicinski wrote:
>>> On Sat, 30 Jan 2021 10:33:19 +0800 Chris Mi wrote:
>>>> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>>>> +/* Copyright (c) 2021 Mellanox Technologies. */
>>>> +
>>>> +const struct psample_ops __rcu *psample_ops __read_mostly;
>>>> +EXPORT_SYMBOL_GPL(psample_ops);
>>> Please explain to me how you could possibly have compile tested this
>>> and not caught that it doesn't build.
>> Sorry, I don't understand which issue you are talking about. Do you mean
>> the issue sparse found before or new issues in v5?
> There is no include here now, and it uses EXPORT_SYMBOL_GPL()
> and a bunch of decorators.
>
But there is no errors in 'checks' section of this page:
https://patchwork.kernel.org/project/netdevbpf/patch/20210128014543.521151-1-cmi@nvidia.com/

And I followed kernel test robot guide, I reproduced the errors in v1, 
but I fixed it in v2.

reproduce:

         wgethttps://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross  -O ~/bin/make.cross
         chmod +x ~/bin/make.cross
         # apt-get install sparse
         # sparse version: v0.6.3-212-g56dbccf5-dirty
         #https://github.com/0day-ci/linux/commit/f2df98afc1a1f1809d9e8a178b2d4766cbca8bf7
         git remote add linux-reviewhttps://github.com/0day-ci/linux
         git fetch --no-tags linux-review Chris-Mi/net-psample-Introduce-stubs-to-remove-NIC-driver-dependency/20210127-082451
         git checkout f2df98afc1a1f1809d9e8a178b2d4766cbca8bf7
         # save the attached .config to linux build tree
         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=s390

I didn't see errors even if there is no include.