[PATCH v2 net-next] sfc: Use netdev refcount tracking in struct efx_async_filter_insertion

[PATCH v2 net-next] sfc: Use netdev refcount tracking in struct efx_async_filter_insertion

[YiFei Zhu]

I was debugging some netdev refcount issues in OpenOnload, and one
of the places I was looking at was in the sfc driver. Only
struct efx_async_filter_insertion was not using netdev refcount tracker,
so add it here. GFP_ATOMIC because this code path is called by
ndo_rx_flow_steer which holds RCU.

This patch should be a no-op if !CONFIG_NET_DEV_REFCNT_TRACKER

Signed-off-by: YiFei Zhu <zhuyifei@google.com>
---
v1 -> v2:
- Documented the added field of @net_dev_tracker in the struct
---
 drivers/net/ethernet/sfc/net_driver.h       | 2 ++
 drivers/net/ethernet/sfc/rx_common.c        | 4 ++--
 drivers/net/ethernet/sfc/siena/net_driver.h | 2 ++
 drivers/net/ethernet/sfc/siena/rx_common.c  | 4 ++--
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 620ba6ef3514b..f70a7b7d6345c 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -831,6 +831,7 @@ struct efx_arfs_rule {
 /**
  * struct efx_async_filter_insertion - Request to asynchronously insert a filter
  * @net_dev: Reference to the netdevice
+ * @net_dev_tracker: reference tracker entry for @net_dev
  * @spec: The filter to insert
  * @work: Workitem for this request
  * @rxq_index: Identifies the channel for which this request was made
@@ -838,6 +839,7 @@ struct efx_arfs_rule {
  */
 struct efx_async_filter_insertion {
 	struct net_device *net_dev;
+	netdevice_tracker net_dev_tracker;
 	struct efx_filter_spec spec;
 	struct work_struct work;
 	u16 rxq_index;
diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
index ab358fe13e1df..cb0a98469f099 100644
--- a/drivers/net/ethernet/sfc/rx_common.c
+++ b/drivers/net/ethernet/sfc/rx_common.c
@@ -897,7 +897,7 @@ static void efx_filter_rfs_work(struct work_struct *data)
 
 	/* Release references */
 	clear_bit(slot_idx, &efx->rps_slot_map);
-	dev_put(req->net_dev);
+	netdev_put(req->net_dev, &req->net_dev_tracker);
 }
 
 int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
@@ -989,7 +989,7 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
 	}
 
 	/* Queue the request */
-	dev_hold(req->net_dev = net_dev);
+	netdev_hold(req->net_dev = net_dev, &req->net_dev_tracker, GFP_ATOMIC);
 	INIT_WORK(&req->work, efx_filter_rfs_work);
 	req->rxq_index = rxq_index;
 	req->flow_id = flow_id;
diff --git a/drivers/net/ethernet/sfc/siena/net_driver.h b/drivers/net/ethernet/sfc/siena/net_driver.h
index 9785eff10607b..2be3bad3c9933 100644
--- a/drivers/net/ethernet/sfc/siena/net_driver.h
+++ b/drivers/net/ethernet/sfc/siena/net_driver.h
@@ -753,6 +753,7 @@ struct efx_arfs_rule {
 /**
  * struct efx_async_filter_insertion - Request to asynchronously insert a filter
  * @net_dev: Reference to the netdevice
+ * @net_dev_tracker: reference tracker entry for @net_dev
  * @spec: The filter to insert
  * @work: Workitem for this request
  * @rxq_index: Identifies the channel for which this request was made
@@ -760,6 +761,7 @@ struct efx_arfs_rule {
  */
 struct efx_async_filter_insertion {
 	struct net_device *net_dev;
+	netdevice_tracker net_dev_tracker;
 	struct efx_filter_spec spec;
 	struct work_struct work;
 	u16 rxq_index;
diff --git a/drivers/net/ethernet/sfc/siena/rx_common.c b/drivers/net/ethernet/sfc/siena/rx_common.c
index 082e35c6caaae..450e6d435d5e1 100644
--- a/drivers/net/ethernet/sfc/siena/rx_common.c
+++ b/drivers/net/ethernet/sfc/siena/rx_common.c
@@ -888,7 +888,7 @@ static void efx_filter_rfs_work(struct work_struct *data)
 
 	/* Release references */
 	clear_bit(slot_idx, &efx->rps_slot_map);
-	dev_put(req->net_dev);
+	netdev_put(req->net_dev, &req->net_dev_tracker);
 }
 
 int efx_siena_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
@@ -980,7 +980,7 @@ int efx_siena_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
 	}
 
 	/* Queue the request */
-	dev_hold(req->net_dev = net_dev);
+	netdev_hold(req->net_dev = net_dev, &req->net_dev_tracker, GFP_ATOMIC);
 	INIT_WORK(&req->work, efx_filter_rfs_work);
 	req->rxq_index = rxq_index;
 	req->flow_id = flow_id;
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2 net-next] sfc: Use netdev refcount tracking in struct efx_async_filter_insertion

[Edward Cree]

On 17/12/2024 22:47, YiFei Zhu wrote:
> I was debugging some netdev refcount issues in OpenOnload, and one
> of the places I was looking at was in the sfc driver. Only
> struct efx_async_filter_insertion was not using netdev refcount tracker,
> so add it here. GFP_ATOMIC because this code path is called by
> ndo_rx_flow_steer which holds RCU.
> 
> This patch should be a no-op if !CONFIG_NET_DEV_REFCNT_TRACKER
> 
> Signed-off-by: YiFei Zhu <zhuyifei@google.com>
> ---
> v1 -> v2:
> - Documented the added field of @net_dev_tracker in the struct

Please do not post new versions of a patch within 24 hours, see
 https://docs.kernel.org/process/maintainer-netdev.html#resending-after-review

> @@ -989,7 +989,7 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
>  	}
>  
>  	/* Queue the request */
> -	dev_hold(req->net_dev = net_dev);
> +	netdev_hold(req->net_dev = net_dev, &req->net_dev_tracker, GFP_ATOMIC);

This line becomes sufficiently complex that the assignment to
 req->net_dev should be separated out into its own statement.
(And the same thing in the siena equivalent.)

Other than that the direction of this patch looks okay.  Have you
 tested it with the kconfig enabled?

Re: [PATCH v2 net-next] sfc: Use netdev refcount tracking in struct efx_async_filter_insertion

[YiFei Zhu]

On Wed, Dec 18, 2024 at 2:09 AM Edward Cree <ecree.xilinx@gmail.com> wrote:
>
> On 17/12/2024 22:47, YiFei Zhu wrote:
> > I was debugging some netdev refcount issues in OpenOnload, and one
> > of the places I was looking at was in the sfc driver. Only
> > struct efx_async_filter_insertion was not using netdev refcount tracker,
> > so add it here. GFP_ATOMIC because this code path is called by
> > ndo_rx_flow_steer which holds RCU.
> >
> > This patch should be a no-op if !CONFIG_NET_DEV_REFCNT_TRACKER
> >
> > Signed-off-by: YiFei Zhu <zhuyifei@google.com>
> > ---
> > v1 -> v2:
> > - Documented the added field of @net_dev_tracker in the struct
>
> Please do not post new versions of a patch within 24 hours, see
>  https://docs.kernel.org/process/maintainer-netdev.html#resending-after-review

My apologies. I noticed the missing docstring while backporting the
refcount tracking in tc_encap_actions.c to OpenOnload, so I thought
I'd send a revision to avoid inconsistencies, and to avoid wasting
reviewer time to point out something that I've already noticed. Was
not aware of the 24h rule, sorry.

> > @@ -989,7 +989,7 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
> >       }
> >
> >       /* Queue the request */
> > -     dev_hold(req->net_dev = net_dev);
> > +     netdev_hold(req->net_dev = net_dev, &req->net_dev_tracker, GFP_ATOMIC);
>
> This line becomes sufficiently complex that the assignment to
>  req->net_dev should be separated out into its own statement.
> (And the same thing in the siena equivalent.)

Will fix it in v3.

> Other than that the direction of this patch looks okay.  Have you
>  tested it with the kconfig enabled?

Only a compile test. It was a blanket change from dev_hold ->
netdev_hold in OpenOnload to wherever it made obvious sense. It was
only after, that I found that it was the driver for solarflare NIC and
was told it was a copy of the in-kernel driver. Since I don't have a
solarflare NIC on hand, the code path is not exercised.

YiFei Zhu