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
