Message-ID: <20210211215952.GA374617@shredder.lan>
Date:   Thu, 11 Feb 2021 23:59:52 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Saeed Mahameed <saeed@...nel.org>
Cc:     Leon Romanovsky <leon@...nel.org>, Chris Mi <cmi@...dia.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        jiri@...dia.com, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH net-next v4] net: psample: Introduce stubs to remove NIC
 driver dependency

On Wed, Feb 10, 2021 at 09:09:41PM -0800, Saeed Mahameed wrote:
> 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.

You are again explaining what are the implications of the dependency,
but you are not explaining why it is the end of the world for you. All I
hear are hypotheticals. Dependencies are also a common practice in the
kernel and mlx5 has a few of its own (I see that pci_hyperv_intf was
loaded by mlx5_core on my system, for example).

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

If you want to move forward with patch, then I ask that you provide a
proper commit message with all the information that was exchanged in
this thread so that multiple people wouldn't need to milk it again upon
re-submission. For example:

"
The tc-sample action sends sampled packets to the psample module which
encapsulates them in generic netlink messages along with associated
metadata and emits notifications to user space.

Device drivers that offload this action are expected to report sampled
packets directly to the psample module by calling
psample_sample_packet(). This creates a dependency between these drivers
and the psample module.

While we are not aware of a problem this dependency can create, we
prefer to avoid it due to past experience with other dependencies. For
example, we discovered that a dependency of mlx5_core on nf_conntrack
will result in mlx5_core being unloaded upon a restart of the firewalld
service. This is because the firewalld service iterates over all the
dependants of nf_conntrack and unloads them so that it could eventually
unload nf_conntrack [1]. In addition, the psample module is only needed
in a small subset of deployments.

Therefore, avoid this dependency by doing XYZ. This is a common way to
reduce dependencies and employed by XYZ, for example.

Note that while the psample module will not be loaded upon the loading
of offloading drivers, it will be loaded by act_sample, which depends on
it. And since drivers offload the act_sample action, psample will be
loaded when needed.

Encapsulating the sampling code in a driver with a config option and
making it depend on psample will result in the psample module being
loaded in most cases given that some distributions blindly enable all
config options.

[1] https://github.com/firewalld/firewalld/blob/master/src/firewall/core/modules.py#L97
"

I also ask that this patch will be routed via the mlxsw queue. Few
reasons:

1. mlxsw already depends on psample while mlx5 does not. Therefore, this
patch needs to take care of mlxsw first. There is no reason to call into
psample differently from different drivers

2. We are about to queue changes (for 5.13) to psample that are going to
conflict with this patch. To avoid the conflict, I want to queue this
patch on top of these changes. The changes also contain selftests which
will provide better test coverage for this patch

Thanks

Powered by blists - more mailing lists