[iwl-next,v1] ice: do not init struct ice_adapter more times than needed

Commit Message

Przemek Kitszel June 17, 2024, 1:24 p.m. UTC

Allocate and initialize struct ice_adapter object only once per physical
card instead of once per port. This is not a big deal by now, but we want
to extend this struct more and more in the near future. Our plans include
PTP stuff and a devlink instance representing whole-device/physical card.

Transactions requiring to be sleep-able (like those doing user (here ice)
memory allocation) must be performed with an additional (on top of xarray)
mutex. Adding it here removes need to xa_lock() manually.

Since this commit is a reimplementation of ice_adapter_get(), a rather new
scoped_guard() wrapper for locking is used to simplify the logic.

It's worth to mention that xa_insert() use gives us both slot reservation
and checks if it is already filled, what simplifies code a tiny	bit.

Reviewed-by: Wojciech Drewek <[e-mail]>
Signed-off-by: Przemek Kitszel <[e-mail]>
---
 drivers/net/ethernet/intel/ice/ice_adapter.c | 60 +++++++++-----------
 1 file changed, 28 insertions(+), 32 deletions(-)


base-commit: 37cf9b0b18612fcb52a819518074e4a0beabe29a

Comments

Simon Horman June 18, 2024, 1:33 p.m. UTC | #1

On Mon, Jun 17, 2024 at 03:24:07PM +0200, Przemek Kitszel wrote:
> Allocate and initialize struct ice_adapter object only once per physical
> card instead of once per port. This is not a big deal by now, but we want
> to extend this struct more and more in the near future. Our plans include
> PTP stuff and a devlink instance representing whole-device/physical card.
> 
> Transactions requiring to be sleep-able (like those doing user (here ice)
> memory allocation) must be performed with an additional (on top of xarray)
> mutex. Adding it here removes need to xa_lock() manually.
> 
> Since this commit is a reimplementation of ice_adapter_get(), a rather new
> scoped_guard() wrapper for locking is used to simplify the logic.
> 
> It's worth to mention that xa_insert() use gives us both slot reservation
> and checks if it is already filled, what simplifies code a tiny	bit.
> 
> Reviewed-by: Wojciech Drewek <[e-mail]>
> Signed-off-by: Przemek Kitszel <[e-mail]>

Reviewed-by: Simon Horman <[e-mail]>

Michal Schmidt June 19, 2024, 1:08 p.m. UTC | #2

On Mon, Jun 17, 2024 at 3:25 PM Przemek Kitszel
<[e-mail]> wrote:
> Allocate and initialize struct ice_adapter object only once per physical
> card instead of once per port. This is not a big deal by now, but we want
> to extend this struct more and more in the near future. Our plans include
> PTP stuff and a devlink instance representing whole-device/physical card.
>
> Transactions requiring to be sleep-able (like those doing user (here ice)
> memory allocation) must be performed with an additional (on top of xarray)
> mutex. Adding it here removes need to xa_lock() manually.
>
> Since this commit is a reimplementation of ice_adapter_get(), a rather new
> scoped_guard() wrapper for locking is used to simplify the logic.
>
> It's worth to mention that xa_insert() use gives us both slot reservation
> and checks if it is already filled, what simplifies code a tiny bit.
>
> Reviewed-by: Wojciech Drewek <[e-mail]>
> Signed-off-by: Przemek Kitszel <[e-mail]>
> ---
>  drivers/net/ethernet/intel/ice/ice_adapter.c | 60 +++++++++-----------
>  1 file changed, 28 insertions(+), 32 deletions(-)

Reviewed-by: Michal Schmidt <[e-mail]>

Pucha, HimasekharX Reddy June 24, 2024, 6:50 a.m. UTC | #3

> -----Original Message-----
> From: Intel-wired-lan <[e-mail]> On Behalf Of Przemek Kitszel
>Sent: Monday, June 17, 2024 6:54 PM
> To: [e-mail]
> Cc: Jiri Pirko <[e-mail]>; Temerkhanov, Sergey <[e-mail]>; [e-mail]; Czapnik, Lukasz <[e-mail]>; Drewek, Wojciech <[e-mail]>; Nguyen, Anthony L <[e-mail]>; Kitszel, Przemyslaw <[e-mail]>; Keller, Jacob E <[e-mail]>; Jakub Kicinski <[e-mail]>
> Subject: [Intel-wired-lan] [PATCH iwl-next v1] ice: do not init struct ice_adapter more times than needed
>
> Allocate and initialize struct ice_adapter object only once per physical card instead of once per port. This is not a big deal by now, but we want to extend this struct more and more in the near future. Our plans include PTP stuff and a devlink instance representing whole-device/physical card.
>
> Transactions requiring to be sleep-able (like those doing user (here ice) memory allocation) must be performed with an additional (on top of xarray) mutex. Adding it here removes need to xa_lock() manually.
>
> Since this commit is a reimplementation of ice_adapter_get(), a rather new
> scoped_guard() wrapper for locking is used to simplify the logic.
>
> It's worth to mention that xa_insert() use gives us both slot reservation
> and checks if it is already filled, what simplifies code a tiny	bit.
>
> Reviewed-by: Wojciech Drewek <[e-mail]>
> Signed-off-by: Przemek Kitszel <[e-mail]>
> ---
>  drivers/net/ethernet/intel/ice/ice_adapter.c | 60 +++++++++-----------
>  1 file changed, 28 insertions(+), 32 deletions(-)
>

Tested-by: Pucha Himasekhar Reddy <[e-mail]> (A Contingent worker at Intel)

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
index 52d15ef7f4b1..ad84d8ad49a6 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.c
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
@@ -11,6 +11,7 @@ 
 #include "ice_adapter.h"
 
 static DEFINE_XARRAY(ice_adapters);
+static DEFINE_MUTEX(ice_adapters_mutex);
 
 /* PCI bus number is 8 bits. Slot is 5 bits. Domain can have the rest. */
 #define INDEX_FIELD_DOMAIN GENMASK(BITS_PER_LONG - 1, 13)
@@ -47,8 +48,6 @@  static void ice_adapter_free(struct ice_adapter *adapter)
 	kfree(adapter);
 }
 
-DEFINE_FREE(ice_adapter_free, struct ice_adapter*, if (_T) ice_adapter_free(_T))
-
 /**
  * ice_adapter_get - Get a shared ice_adapter structure.
  * @pdev: Pointer to the pci_dev whose driver is getting the ice_adapter.
@@ -64,53 +63,50 @@  DEFINE_FREE(ice_adapter_free, struct ice_adapter*, if (_T) ice_adapter_free(_T))
  */
 struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev)
 {
-	struct ice_adapter *ret, __free(ice_adapter_free) *adapter = NULL;
 	unsigned long index = ice_adapter_index(pdev);
-
-	adapter = ice_adapter_new();
-	if (!adapter)
-		return ERR_PTR(-ENOMEM);
-
-	xa_lock(&ice_adapters);
-	ret = __xa_cmpxchg(&ice_adapters, index, NULL, adapter, GFP_KERNEL);
-	if (xa_is_err(ret)) {
-		ret = ERR_PTR(xa_err(ret));
-		goto unlock;
-	}
-	if (ret) {
-		refcount_inc(&ret->refcount);
-		goto unlock;
+	struct ice_adapter *adapter;
+	int err;
+
+	scoped_guard(mutex, &ice_adapters_mutex) {
+		err = xa_insert(&ice_adapters, index, NULL, GFP_KERNEL);
+		if (err == -EBUSY) {
+			adapter = xa_load(&ice_adapters, index);
+			refcount_inc(&adapter->refcount);
+			return adapter;
+		}
+		if (err)
+			return ERR_PTR(err);
+
+		adapter = ice_adapter_new();
+		if (!adapter)
+			return ERR_PTR(-ENOMEM);
+		xa_store(&ice_adapters, index, adapter, GFP_KERNEL);
 	}
-	ret = no_free_ptr(adapter);
-unlock:
-	xa_unlock(&ice_adapters);
-	return ret;
+	return adapter;
 }
 
 /**
  * ice_adapter_put - Release a reference to the shared ice_adapter structure.
  * @pdev: Pointer to the pci_dev whose driver is releasing the ice_adapter.
  *
  * Releases the reference to ice_adapter previously obtained with
  * ice_adapter_get.
  *
- * Context: Any.
+ * Context: Process, may sleep.
  */
 void ice_adapter_put(const struct pci_dev *pdev)
 {
 	unsigned long index = ice_adapter_index(pdev);
 	struct ice_adapter *adapter;
 
-	xa_lock(&ice_adapters);
-	adapter = xa_load(&ice_adapters, index);
-	if (WARN_ON(!adapter))
-		goto unlock;
+	scoped_guard(mutex, &ice_adapters_mutex) {
+		adapter = xa_load(&ice_adapters, index);
+		if (WARN_ON(!adapter))
+			return;
+		if (!refcount_dec_and_test(&adapter->refcount))
+			return;
 
-	if (!refcount_dec_and_test(&adapter->refcount))
-		goto unlock;
-
-	WARN_ON(__xa_erase(&ice_adapters, index) != adapter);
+		WARN_ON(xa_erase(&ice_adapters, index) != adapter);
+	}
 	ice_adapter_free(adapter);
-unlock:
-	xa_unlock(&ice_adapters);
 }