| Message ID | 20241011183549.1581021-2-daniel.zahka@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 42dc431f5d0ea9e9c9caf74dcb5b290ce4dd80b4 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool: rss: track rss ctx busy from core | expand |
On 11/10/2024 19:35, Daniel Zahka wrote: > A list of active > ntuple filters and their associated rss contexts can be compiled by > querying a device's ethtool_ops.get_rxnfc. This patch checks to see if > any ntuple filters are referencing an rss context during context > deletion, and prevents the deletion if the requested context is still > in use. Imho it would make more sense to add core tracking of ntuple filters, along with a refcount on the rss context. That way context deletion just has to check the count is zero. -ed
On Mon, 14 Oct 2024 11:10:33 +0100 Edward Cree wrote: > On 11/10/2024 19:35, Daniel Zahka wrote: > > A list of active > > ntuple filters and their associated rss contexts can be compiled by > > querying a device's ethtool_ops.get_rxnfc. This patch checks to see if > > any ntuple filters are referencing an rss context during context > > deletion, and prevents the deletion if the requested context is still > > in use. > > Imho it would make more sense to add core tracking of ntuple > filters, along with a refcount on the rss context. That way > context deletion just has to check the count is zero. True... This is my least favorite kind of situation, where the posted code is clearly much better than the status quo, but even better solution is possible. So question becomes do we push for the optimal solution and potentially get neither or do we apply what's posted and hope the better one will still be delivered later..
On 10/14/24 6:10 AM, Edward Cree wrote: > Imho it would make more sense to add core tracking of ntuple > filters, along with a refcount on the rss context. That way > context deletion just has to check the count is zero. > > -ed That sounds good to me. Is that something you are planning on sending patches for?
On 15/10/2024 17:31, Daniel Zahka wrote: > On 10/14/24 6:10 AM, Edward Cree wrote: >> Imho it would make more sense to add core tracking of ntuple >> filters, along with a refcount on the rss context. That way >> context deletion just has to check the count is zero. >> >> -ed > > That sounds good to me. Is that something you are planning on sending patches for? I'm afraid I don't have the bandwidth to do it any time soon. If you aren't able to take this on, I'm okay with your original approach to get the issue fixed; I just wanted to ensure the 'better' solution was considered if you do have the time for it.
On 10/16/24 12:23 PM, Edward Cree wrote: > On 15/10/2024 17:31, Daniel Zahka wrote: >> On 10/14/24 6:10 AM, Edward Cree wrote: >>> Imho it would make more sense to add core tracking of ntuple >>> filters, along with a refcount on the rss context. That way >>> context deletion just has to check the count is zero. >>> >>> -ed >> That sounds good to me. Is that something you are planning on sending patches for? > I'm afraid I don't have the bandwidth to do it any time soon. > If you aren't able to take this on, I'm okay with your original > approach to get the issue fixed; I just wanted to ensure the > 'better' solution was considered if you do have the time for it. Understood. I don't have enough bandwidth to commit to implementing it soon either.
On 10/16/24 18:50, Daniel Zahka wrote: > > On 10/16/24 12:23 PM, Edward Cree wrote: >> On 15/10/2024 17:31, Daniel Zahka wrote: >>> On 10/14/24 6:10 AM, Edward Cree wrote: >>>> Imho it would make more sense to add core tracking of ntuple >>>> filters, along with a refcount on the rss context. That way >>>> context deletion just has to check the count is zero. >>>> >>>> -ed >>> That sounds good to me. Is that something you are planning on sending patches for? >> I'm afraid I don't have the bandwidth to do it any time soon. >> If you aren't able to take this on, I'm okay with your original >> approach to get the issue fixed; I just wanted to ensure the >> 'better' solution was considered if you do have the time for it. > Understood. I don't have enough bandwidth to commit to implementing it > soon either. I understand the chances to have a refcount based implementation soon are zero, and I could not find any obvious problem with the proposed solution, I'm going to apply this series as-is. Cheers, Paolo
diff --git a/net/ethtool/common.c b/net/ethtool/common.c index dd345efa114b..0d62363dbd9d 100644 --- a/net/ethtool/common.c +++ b/net/ethtool/common.c @@ -684,6 +684,54 @@ int ethtool_check_max_channel(struct net_device *dev, return 0; } +int ethtool_check_rss_ctx_busy(struct net_device *dev, u32 rss_context) +{ + const struct ethtool_ops *ops = dev->ethtool_ops; + struct ethtool_rxnfc *info; + int rc, i, rule_cnt; + + if (!ops->get_rxnfc) + return 0; + + rule_cnt = ethtool_get_rxnfc_rule_count(dev); + if (!rule_cnt) + return 0; + + if (rule_cnt < 0) + return -EINVAL; + + info = kvzalloc(struct_size(info, rule_locs, rule_cnt), GFP_KERNEL); + if (!info) + return -ENOMEM; + + info->cmd = ETHTOOL_GRXCLSRLALL; + info->rule_cnt = rule_cnt; + rc = ops->get_rxnfc(dev, info, info->rule_locs); + if (rc) + goto out_free; + + for (i = 0; i < rule_cnt; i++) { + struct ethtool_rxnfc rule_info = { + .cmd = ETHTOOL_GRXCLSRULE, + .fs.location = info->rule_locs[i], + }; + + rc = ops->get_rxnfc(dev, &rule_info, NULL); + if (rc) + goto out_free; + + if (rule_info.fs.flow_type & FLOW_RSS && + rule_info.rss_context == rss_context) { + rc = -EBUSY; + goto out_free; + } + } + +out_free: + kvfree(info); + return rc; +} + int ethtool_check_ops(const struct ethtool_ops *ops) { if (WARN_ON(ops->set_coalesce && !ops->supported_coalesce_params)) diff --git a/net/ethtool/common.h b/net/ethtool/common.h index d55d5201b085..4a2de3ce7354 100644 --- a/net/ethtool/common.h +++ b/net/ethtool/common.h @@ -47,6 +47,7 @@ bool convert_legacy_settings_to_link_ksettings( int ethtool_check_max_channel(struct net_device *dev, struct ethtool_channels channels, struct genl_info *info); +int ethtool_check_rss_ctx_busy(struct net_device *dev, u32 rss_context); int __ethtool_get_ts_info(struct net_device *dev, struct kernel_ethtool_ts_info *info); extern const struct ethtool_phy_ops *ethtool_phy_ops; diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index 04b34dc6b369..5cc131cdb1bc 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -1462,6 +1462,13 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, mutex_lock(&dev->ethtool->rss_lock); locked = true; } + + if (rxfh.rss_context && rxfh_dev.rss_delete) { + ret = ethtool_check_rss_ctx_busy(dev, rxfh.rss_context); + if (ret) + goto out; + } + if (create) { if (rxfh_dev.rss_delete) { ret = -EINVAL;
ntuple filters can specify an rss context to use for packet hashing and queue selection. When a filter is referencing an rss context, it should be invalid for that context to be deleted. A list of active ntuple filters and their associated rss contexts can be compiled by querying a device's ethtool_ops.get_rxnfc. This patch checks to see if any ntuple filters are referencing an rss context during context deletion, and prevents the deletion if the requested context is still in use. Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com> --- net/ethtool/common.c | 48 ++++++++++++++++++++++++++++++++++++++++++++ net/ethtool/common.h | 1 + net/ethtool/ioctl.c | 7 +++++++ 3 files changed, 56 insertions(+)