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

[Saeed Mahameed <saeed@kernel.org>]

On Tue, 2021-02-09 at 08:47 +0200, Leon Romanovsky wrote:
> On Mon, Feb 08, 2021 at 07:07:35PM +0200, Ido Schimmel wrote:
> > On Mon, Feb 08, 2021 at 11:07:02AM +0200, Leon Romanovsky wrote:
> > 
> > 

[...]

> > I don't know. What are they complaining about? That psample needs
> > to be
> > installed for mlx5_core to be loaded? How come the rest of the
> > dependencies are installed?
> 
> The psample module was first dependency that caught our attention. It
> is
> here as an example of such not-needed dependency. Like Saeed said, we
> are
> interested in more general solution that will allow us to use
> external
> modules in fully dynamic mode.
> 
> Internally, as a preparation to the submission of mlx5 code that used
> nf_conntrack,
> we found that restart of firewald service will bring down our
> mlx5_core driver, because
> of such dependency.
> 
> So to answer on your question, HPC didn't complain yet, but we don't
> have any plans
> to wait till they complain.
> 
> > 
> > Or are they complaining about the size / memory footprint of
> > psample?
> > Because then they should first check mlx5_core when all of its
> > options
> > are blindly enabled as part of a distribution config.
> 
> You are too focused on psample, while Saeed and I are saying more
> general statement "I prefer to have 0 dependency on external modules
> in a HW driver."
> 
> > 
> > AFAICS, mlx5 still does not have any code that uses psample. You
> > can
> > wrap it in a config option and keep the weak dependency on psample.
> > Something like:
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
> > b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
> > index ad45d20f9d44..d17d03d8cc8b 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
> > @@ -104,6 +104,15 @@ config MLX5_TC_CT
> > 
> >           If unsure, set to Y
> > 
> > +config MLX5_TC_SAMPLE
> > +       bool "MLX5 TC sample offload support"
> > +       depends on MLX5_CLS_ACT
> > +       depends on PSAMPLE || PSAMPLE=n
> > +       default n
> > +       help
> > +         Say Y here if you want to support offloading tc rules
> > that use sample
> > +          action.
> > +
> 

This won't solve anything other than compilation time dependency
between built-in modules to external modules, this is not the case.

our case is when both mlx5 and psample are modules, you can't load mlx5
without loading psample, even if you are not planning to use psample or
mlx5 psample features, which is 99.99% of the cases.

What we are asking for here is not new, and is a common practice in
netdev stack

see :
udp_tunnel_nic_ops
netfilter is full of these, see nf_ct_hook..

I don't see anything wrong with either repeating this practice for any
module or having some sort of a generic proxy in the built-in netdev
layer..