Message-ID: <f8320042b1b0d60f8885e7c62760ee4fca0b89b2.camel@kernel.org>
Date:   Thu, 11 Feb 2021 16:41:45 -0800
From:   Saeed Mahameed <saeed@...nel.org>
To:     Ido Schimmel <idosch@...sch.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 Thu, 2021-02-11 at 23:59 +0200, Ido Schimmel wrote:
> 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).
> 

Very great example actually, the reason there is pci_hyperv_intf in
first place is exactly to avoid hard dependency between mlx5_core and
pci_hyperv.ko so they came up with pci_hyperv_intf which is a thin
layer between pci_hyperv and mlx5_core, microsoft insisted at the time
that pci_hyperv_intf can be a module (I really don't know why
though) ..

if there wasn't  pci_hyperv_intf then pci_hyperv would be a direct
dependency of mlx5_core, the problem s that pci_hperv can only be
loaded on a hyperv VM.. if you try to load it on any different host it
will fail, thus mlx5 can never load on any system where pci_hyperv.ko
is enabled as CONFIG option but it is not a hyperv VM..

Anyway the point is clear and very straight forward:

A hw_driver.ko should never depend on feature_x.ko, because who knows
what dependencies feature_x.ko are hiding behind or what pre-conditions
feature_x.ko can impose.


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

Apparently you didn't milk this thread well enough, we already shut the
door on this patch weeks ago :)

But We are still discussing the dependency claim, not related to
psample..
We have some features lined up with harder dependencies than psample.

Again as i previously said, I will try to come up with a unified
universal approach with the necessary documentation and explanations.


> 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

Thanks, will keep this in mind.

Powered by blists - more mailing lists