| Message ID | 20210130023319.32560-1-cmi@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v5] net: psample: Introduce stubs to remove NIC driver dependency | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: jhs@mojatatu.com yotam.gi@gmail.com jiri@resnulli.us davem@davemloft.net xiyou.wangcong@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 527 this patch: 9 |
| netdev/kdoc | success | Errors and warnings before: 1 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 527 this patch: 9 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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.
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 >
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.
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 >>
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.
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.
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 :)
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.
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);