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:

quoted

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.

quoted

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.