Re: [PATCH net-next v13 2/4] ethtool: provide customized dim profile management

Re: [PATCH net-next v13 2/4] ethtool: provide customized dim profile management

[kernel test robot]

Hi Heng,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Heng-Qi/linux-dim-move-useful-macros-to-h-file/20240509-125007
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240509044747.101237-3-hengqi%40linux.alibaba.com
patch subject: [PATCH net-next v13 2/4] ethtool: provide customized dim profile management
config: arm-randconfig-002-20240510 (https://download.01.org/0day-ci/archive/20240510/202405100654.5PbLQXnL-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project b910bebc300dafb30569cecc3017b446ea8eafa0)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240510/202405100654.5PbLQXnL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405100654.5PbLQXnL-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: lockdep_rtnl_is_held
   >>> referenced by net_dim.c
   >>>               lib/dim/net_dim.o:(net_dim_free_irq_moder) in archive vmlinux.a
   >>> referenced by net_dim.c
   >>>               lib/dim/net_dim.o:(net_dim_free_irq_moder) in archive vmlinux.a

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v13 2/4] ethtool: provide customized dim profile management

[Heng Qi]

On Fri, 10 May 2024 07:09:52 +0800, kernel test robot <lkp@intel.com> wrote:
> Hi Heng,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on net-next/main]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Heng-Qi/linux-dim-move-useful-macros-to-h-file/20240509-125007
> base:   net-next/main
> patch link:    https://lore.kernel.org/r/20240509044747.101237-3-hengqi%40linux.alibaba.com
> patch subject: [PATCH net-next v13 2/4] ethtool: provide customized dim profile management
> config: arm-randconfig-002-20240510 (https://download.01.org/0day-ci/archive/20240510/202405100654.5PbLQXnL-lkp@intel.com/config)
> compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project b910bebc300dafb30569cecc3017b446ea8eafa0)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240510/202405100654.5PbLQXnL-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202405100654.5PbLQXnL-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
> >> ld.lld: error: undefined symbol: lockdep_rtnl_is_held

This failed use case seems to come from this series triggering a problem that
has not been triggered historically, namely lockdep_rtnl_is_held() is not called
in an environment where CONFIG_NET is not configured and CONFIG_PROVE_LOCKING is
configured:
  If CONFIG_PROVE_LOCKING is configured as Y and CONFIG_NET is n, then
  lockdep_rtnl_is_held is in an undefined state at this time.

So I think we should declare "CONFIG_PROVE_LOCKING depends on CONFIG_NET".
How do you think?

Thanks!

>    >>> referenced by net_dim.c
>    >>>               lib/dim/net_dim.o:(net_dim_free_irq_moder) in archive vmlinux.a
>    >>> referenced by net_dim.c
>    >>>               lib/dim/net_dim.o:(net_dim_free_irq_moder) in archive vmlinux.a
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v13 2/4] ethtool: provide customized dim profile management

[Jakub Kicinski]

On Mon, 13 May 2024 00:36:58 +0800 Heng Qi wrote:
> This failed use case seems to come from this series triggering a problem that
> has not been triggered historically, namely lockdep_rtnl_is_held() is not called
> in an environment where CONFIG_NET is not configured and CONFIG_PROVE_LOCKING is
> configured:
>   If CONFIG_PROVE_LOCKING is configured as Y and CONFIG_NET is n, then
>   lockdep_rtnl_is_held is in an undefined state at this time.
> 
> So I think we should declare "CONFIG_PROVE_LOCKING depends on CONFIG_NET".
> How do you think?

Doesn't sound right, can we instead make building lib/dim/net_dim.c
dependent on CONFIG_NET? Untested but I'm thinking something like:

diff --git a/lib/dim/Makefile b/lib/dim/Makefile
index c4cc4026c451..c02c306e2975 100644
--- a/lib/dim/Makefile
+++ b/lib/dim/Makefile
@@ -4,4 +4,8 @@
 
 obj-$(CONFIG_DIMLIB) += dimlib.o
 
-dimlib-objs := dim.o net_dim.o rdma_dim.o
+dimlib-objs := dim.o rdma_dim.o
+
+ifeq ($(CONFIG_NET),y)
+dimlib-objs += net_dim.o
+endif

Re: [PATCH net-next v13 2/4] ethtool: provide customized dim profile management

[Heng Qi]

On Mon, 13 May 2024 07:22:49 -0700, Jakub Kicinski <kuba@kernel.org> wrote:
> On Mon, 13 May 2024 00:36:58 +0800 Heng Qi wrote:
> > This failed use case seems to come from this series triggering a problem that
> > has not been triggered historically, namely lockdep_rtnl_is_held() is not called
> > in an environment where CONFIG_NET is not configured and CONFIG_PROVE_LOCKING is
> > configured:
> >   If CONFIG_PROVE_LOCKING is configured as Y and CONFIG_NET is n, then
> >   lockdep_rtnl_is_held is in an undefined state at this time.
> > 
> > So I think we should declare "CONFIG_PROVE_LOCKING depends on CONFIG_NET".
> > How do you think?
> 
> Doesn't sound right, `can we instead make building lib/dim/net_dim.c

Why? IIUC, the reason is that if CONFIG_NET is not set to Y, the net/core
directory will not be compiled, so the lockdep_rtnl_is_held symbol is not
present.

> dependent on CONFIG_NET? Untested but I'm thinking something like:
> 
> diff --git a/lib/dim/Makefile b/lib/dim/Makefile
> index c4cc4026c451..c02c306e2975 100644
> --- a/lib/dim/Makefile
> +++ b/lib/dim/Makefile
> @@ -4,4 +4,8 @@
>  
>  obj-$(CONFIG_DIMLIB) += dimlib.o
>  
> -dimlib-objs := dim.o net_dim.o rdma_dim.o
> +dimlib-objs := dim.o rdma_dim.o
> +
> +ifeq ($(CONFIG_NET),y)
> +dimlib-objs += net_dim.o
> +endif

1. This is unlikely to work if the kernel is configured as[1]:

[1] kernel configuration
CONFIG_NET=n, CONFIG_ETHTOOL_NETLINK=n, CONFIG_PROVE_LOCKING=y,
(CONFIG_FSL_MC_DPIO=y && CONFIG_FSL_MC_BUS=y) select CONFIG_DIMLIB=y.


Then, because CONFIG_NET is not enabled, so there is no net_dim.o,
the following warning appears:

ld.lld: error: undefined symbol: net_dim_get_rx_moderation
referenced by dpio-service.c
drivers/soc/fsl/dpio/dpio-service.o:(dpaa2_io_dim_work) in archive vmlinux.a

ld.lld: error: undefined symbol: net_dim
referenced by dpio-service.c
drivers/soc/fsl/dpio/dpio-service.o:(dpaa2_io_update_net_dim) in archive vmlinux.a

2. If we declare "CONFIG_DIMLIB depends on CONFIG_NET",
if the configuration is still [1]:
Then the result is:
CONFIG_DIMLIB=Y (selected by CONFIG_FSL_MC_DPIO=y && CONFIG_FSL_MC_BUS=y),
CONFIG_NET=n, but we declared "CONFIG_DIMLIB depends on CONFIG_NET",
there is still a compilation error because the lockdep_rtnl_is_held symbol
cannot be found.

3. If we declare "CONFIG_DIMLIB select CONFIG_NET" and kernel configuration is [1],
then a circular dependency warning will appear:

CONFIG_DIMLIB select CONFIG_NET, ETHTOOL_NETLINK=Y(depends on CONFIG_NET)
selects CONFIG_DIMLIB. CONFIG_DIMLIB will select CONFIG_NET...

> 
> 

Re: [PATCH net-next v13 2/4] ethtool: provide customized dim profile management

[Jakub Kicinski]

On Mon, 13 May 2024 22:52:13 +0800 Heng Qi wrote:
> > > So I think we should declare "CONFIG_PROVE_LOCKING depends on CONFIG_NET".
> > > How do you think?  
> > 
> > Doesn't sound right, `can we instead make building lib/dim/net_dim.c  
> 
> Why? IIUC, the reason is that if CONFIG_NET is not set to Y, the net/core
> directory will not be compiled, so the lockdep_rtnl_is_held symbol is not
> present.

Maybe I don't understand what you;re proposing. 
Show an actual diff please.

> > dependent on CONFIG_NET? Untested but I'm thinking something like:
> > 
> > diff --git a/lib/dim/Makefile b/lib/dim/Makefile
> > index c4cc4026c451..c02c306e2975 100644
> > --- a/lib/dim/Makefile
> > +++ b/lib/dim/Makefile
> > @@ -4,4 +4,8 @@
> >  
> >  obj-$(CONFIG_DIMLIB) += dimlib.o
> >  
> > -dimlib-objs := dim.o net_dim.o rdma_dim.o
> > +dimlib-objs := dim.o rdma_dim.o
> > +
> > +ifeq ($(CONFIG_NET),y)
> > +dimlib-objs += net_dim.o
> > +endif  
> 
> 1. This is unlikely to work if the kernel is configured as[1]:
> 
> [1] kernel configuration
> CONFIG_NET=n, CONFIG_ETHTOOL_NETLINK=n, CONFIG_PROVE_LOCKING=y,
> (CONFIG_FSL_MC_DPIO=y && CONFIG_FSL_MC_BUS=y) select CONFIG_DIMLIB=y.
> 
> 
> Then, because CONFIG_NET is not enabled, so there is no net_dim.o,
> the following warning appears:
> 
> ld.lld: error: undefined symbol: net_dim_get_rx_moderation
> referenced by dpio-service.c
> drivers/soc/fsl/dpio/dpio-service.o:(dpaa2_io_dim_work) in archive vmlinux.a
> 
> ld.lld: error: undefined symbol: net_dim
> referenced by dpio-service.c
> drivers/soc/fsl/dpio/dpio-service.o:(dpaa2_io_update_net_dim) in archive vmlinux.a

Simple, dpio-service should depend on NET if it wants NET_DIM

Re: [PATCH net-next v13 2/4] ethtool: provide customized dim profile management

[Heng Qi]

On Mon, 13 May 2024 08:24:12 -0700, Jakub Kicinski <kuba@kernel.org> wrote:
> On Mon, 13 May 2024 22:52:13 +0800 Heng Qi wrote:
> > > > So I think we should declare "CONFIG_PROVE_LOCKING depends on CONFIG_NET".
> > > > How do you think?  
> > > 
> > > Doesn't sound right, `can we instead make building lib/dim/net_dim.c  
> > 
> > Why? IIUC, the reason is that if CONFIG_NET is not set to Y, the net/core
> > directory will not be compiled, so the lockdep_rtnl_is_held symbol is not
> > present.
> 
> Maybe I don't understand what you;re proposing. 
> Show an actual diff please.

Like this:

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 291185f54ee4..fbb20d0d08c1 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1299,7 +1299,7 @@ config LOCK_DEBUGGING_SUPPORT

 config PROVE_LOCKING
        bool "Lock debugging: prove locking correctness"
-       depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT
+       depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT && NET
        select LOCKDEP
        select DEBUG_SPINLOCK
        select DEBUG_MUTEXES if !PREEMPT_RT


And there is:

In include/linux/rtnetlink.h:

#ifdef CONFIG_PROVE_LOCKING
extern bool lockdep_rtnl_is_held(void);
#else
static inline bool lockdep_rtnl_is_held(void)
{
        return true;
}
#endif /* #ifdef CONFIG_PROVE_LOCKING */


In net/core/rtnetlink.c:

#ifdef CONFIG_PROVE_LOCKING
bool lockdep_rtnl_is_held(void)
{
        return lockdep_is_held(&rtnl_mutex);
}
EXPORT_SYMBOL(lockdep_rtnl_is_held);
#endif /* #ifdef CONFIG_PROVE_LOCKING */

Thanks.

> 
> > > dependent on CONFIG_NET? Untested but I'm thinking something like:
> > > 
> > > diff --git a/lib/dim/Makefile b/lib/dim/Makefile
> > > index c4cc4026c451..c02c306e2975 100644
> > > --- a/lib/dim/Makefile
> > > +++ b/lib/dim/Makefile
> > > @@ -4,4 +4,8 @@
> > >  
> > >  obj-$(CONFIG_DIMLIB) += dimlib.o
> > >  
> > > -dimlib-objs := dim.o net_dim.o rdma_dim.o
> > > +dimlib-objs := dim.o rdma_dim.o
> > > +
> > > +ifeq ($(CONFIG_NET),y)
> > > +dimlib-objs += net_dim.o
> > > +endif  
> > 
> > 1. This is unlikely to work if the kernel is configured as[1]:
> > 
> > [1] kernel configuration
> > CONFIG_NET=n, CONFIG_ETHTOOL_NETLINK=n, CONFIG_PROVE_LOCKING=y,
> > (CONFIG_FSL_MC_DPIO=y && CONFIG_FSL_MC_BUS=y) select CONFIG_DIMLIB=y.
> > 
> > 
> > Then, because CONFIG_NET is not enabled, so there is no net_dim.o,
> > the following warning appears:
> > 
> > ld.lld: error: undefined symbol: net_dim_get_rx_moderation
> > referenced by dpio-service.c
> > drivers/soc/fsl/dpio/dpio-service.o:(dpaa2_io_dim_work) in archive vmlinux.a
> > 
> > ld.lld: error: undefined symbol: net_dim
> > referenced by dpio-service.c
> > drivers/soc/fsl/dpio/dpio-service.o:(dpaa2_io_update_net_dim) in archive vmlinux.a
> 
> Simple, dpio-service should depend on NET if it wants NET_DIM

Re: [PATCH net-next v13 2/4] ethtool: provide customized dim profile management

[Jakub Kicinski]

On Mon, 13 May 2024 23:39:04 +0800 Heng Qi wrote:
>  config PROVE_LOCKING
>         bool "Lock debugging: prove locking correctness"
> -       depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT
> +       depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT && NET

We can't make lockdep dependent on NET.
People working on other subsystems should be able to use LOCKDEP 
with minimal builds.

Re: [PATCH net-next v13 2/4] ethtool: provide customized dim profile management

[Heng Qi]

On Mon, 13 May 2024 11:42:33 -0700, Jakub Kicinski <kuba@kernel.org> wrote:
> On Mon, 13 May 2024 23:39:04 +0800 Heng Qi wrote:
> >  config PROVE_LOCKING
> >         bool "Lock debugging: prove locking correctness"
> > -       depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT
> > +       depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT && NET
> 
> We can't make lockdep dependent on NET.
> People working on other subsystems should be able to use LOCKDEP 
> with minimal builds.


Got it. Then I declare "DIMLIB depends on NET" and clean up other places.

One more friendly request, I see net-next is closed today, but our downstream
kernel release deadline is 5.20, so I want to test and release the new v14 today,
is it ok?

Thanks!

Re: [PATCH net-next v13 2/4] ethtool: provide customized dim profile management

[Jakub Kicinski]

On Tue, 14 May 2024 10:08:15 +0800 Heng Qi wrote:
> > We can't make lockdep dependent on NET.
> > People working on other subsystems should be able to use LOCKDEP 
> > with minimal builds.  
> 
> Got it. Then I declare "DIMLIB depends on NET" and clean up other places.

I'm not sure if there's any legit DIM (but not net_dim) user, if there
isn't that SGTM. The RDMA is fine to depend on NET.

> One more friendly request, I see net-next is closed today, but our downstream
> kernel release deadline is 5.20, so I want to test and release the new v14 today,
> is it ok?

You'll need to post as RFC since it can't be applied but you can post
to continue review. Please wait a day or two tho, I'm currently swamped
trying to go thru the PR preparations.

Re: [PATCH net-next v13 2/4] ethtool: provide customized dim profile management

[Andrew Lunn]

> One more friendly request, I see net-next is closed today, but our downstream
> kernel release deadline is 5.20, so I want to test and release the new v14 today,
> is it ok?

5.20? Didn't Linus run out of fingers and thumbs and went from 5.19 to
6.0?

Anyway, this is your internal issue, since netdev only accepts new
features patches for the next kernel release, not stable kernels. If
you are forced to do upstream first, you are going to have to wait
until the next cycle starts, and net-next reopens.

      Andrew