Re: [PATCH net-next v2 1/3] ice: add ice_adapter for shared data across PFs on the same NIC

[Jiri Pirko <jiri@resnulli.us>]

Wed, Mar 06, 2024 at 08:20:33PM CET, mschmidt@redhat.com wrote:
>On Wed, Mar 6, 2024 at 6:00 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> Wed, Mar 06, 2024 at 05:29:05PM CET, mschmidt@redhat.com wrote:
>> >There is a need for synchronization between ice PFs on the same physical
>> >adapter.
>> >
>> >Add a "struct ice_adapter" for holding data shared between PFs of the
>> >same multifunction PCI device. The struct is refcounted - each ice_pf
>> >holds a reference to it.
>> >
>> >Its first use will be for PTP. I expect it will be useful also to
>> >improve the ugliness that is ice_prot_id_tbl.
>> >
>> >Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
>> >---
>> > drivers/net/ethernet/intel/ice/Makefile      |  3 +-
>> > drivers/net/ethernet/intel/ice/ice.h         |  2 +
>> > drivers/net/ethernet/intel/ice/ice_adapter.c | 85 ++++++++++++++++++++
>> > drivers/net/ethernet/intel/ice/ice_adapter.h | 22 +++++
>> > drivers/net/ethernet/intel/ice/ice_main.c    |  8 ++
>> > 5 files changed, 119 insertions(+), 1 deletion(-)
>> > create mode 100644 drivers/net/ethernet/intel/ice/ice_adapter.c
>> > create mode 100644 drivers/net/ethernet/intel/ice/ice_adapter.h
>> >
>> >diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
>> >index cddd82d4ca0f..4fa09c321440 100644
>> >--- a/drivers/net/ethernet/intel/ice/Makefile
>> >+++ b/drivers/net/ethernet/intel/ice/Makefile
>> >@@ -36,7 +36,8 @@ ice-y := ice_main.o  \
>> >        ice_repr.o     \
>> >        ice_tc_lib.o   \
>> >        ice_fwlog.o    \
>> >-       ice_debugfs.o
>> >+       ice_debugfs.o  \
>> >+       ice_adapter.o
>> > ice-$(CONFIG_PCI_IOV) +=      \
>> >       ice_sriov.o             \
>> >       ice_virtchnl.o          \
>> >diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
>> >index 365c03d1c462..1ffecbdd361a 100644
>> >--- a/drivers/net/ethernet/intel/ice/ice.h
>> >+++ b/drivers/net/ethernet/intel/ice/ice.h
>> >@@ -77,6 +77,7 @@
>> > #include "ice_gnss.h"
>> > #include "ice_irq.h"
>> > #include "ice_dpll.h"
>> >+#include "ice_adapter.h"
>> >
>> > #define ICE_BAR0              0
>> > #define ICE_REQ_DESC_MULTIPLE 32
>> >@@ -544,6 +545,7 @@ struct ice_agg_node {
>> >
>> > struct ice_pf {
>> >       struct pci_dev *pdev;
>> >+      struct ice_adapter *adapter;
>> >
>> >       struct devlink_region *nvm_region;
>> >       struct devlink_region *sram_region;
>> >diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
>> >new file mode 100644
>> >index 000000000000..b93b4db4c04c
>> >--- /dev/null
>> >+++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
>> >@@ -0,0 +1,85 @@
>> >+// SPDX-License-Identifier: GPL-2.0-only
>> >+// SPDX-FileCopyrightText: Copyright Red Hat
>> >+
>> >+#include <linux/cleanup.h>
>> >+#include <linux/mutex.h>
>> >+#include <linux/pci.h>
>> >+#include <linux/slab.h>
>> >+#include <linux/xarray.h>
>> >+#include "ice_adapter.h"
>> >+
>> >+static DEFINE_XARRAY(ice_adapters);
>> >+
>> >+static unsigned long ice_adapter_index(const struct pci_dev *pdev)
>> >+{
>> >+      unsigned int domain = pci_domain_nr(pdev->bus);
>> >+
>> >+      WARN_ON((unsigned long)domain >> (BITS_PER_LONG - 13));
>> >+      return ((unsigned long)domain << 13) |
>> >+             ((unsigned long)pdev->bus->number << 5) |
>> >+             PCI_SLOT(pdev->devfn);
>> >+}
>> >+
>> >+static struct ice_adapter *ice_adapter_new(void)
>> >+{
>> >+      struct ice_adapter *a;
>> >+
>> >+      a = kzalloc(sizeof(*a), GFP_KERNEL);
>> >+      if (!a)
>> >+              return NULL;
>> >+
>> >+      refcount_set(&a->refcount, 1);
>> >+
>> >+      return a;
>> >+}
>> >+
>> >+static void ice_adapter_free(struct ice_adapter *a)
>> >+{
>> >+      kfree(a);
>> >+}
>> >+
>> >+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) *a = NULL;
>> >+      unsigned long index = ice_adapter_index(pdev);
>> >+
>> >+      a = ice_adapter_new();
>>
>> Please consider some non-single-letter variable name.
>
>Alright, I can change the name.
>
>> >+      if (!a)
>> >+              return NULL;
>> >+
>> >+      xa_lock(&ice_adapters);
>> >+      ret = __xa_cmpxchg(&ice_adapters, index, NULL, a, GFP_KERNEL);
>>
>> This is atomic section, can't sleep.
>
>It is not atomic. __xa_cmpxchg releases xa_lock before it allocates
>memory, then reacquires it.

Ah, cool.


>
>> >+      if (xa_is_err(ret)) {
>> >+              ret = NULL;
>>
>> Why don't you propagate err through ERR_PTR() ?
>
>It seemed unnecessary. ENOMEM is the only failure that can possibly
>happen. EINVAL could be returned only if attempting to store an
>unaligned pointer, which won't happen here.

Yeah, the point is that you have valid err, you toss it out, the caller
then does:
	adapter = ice_adapter_get(pdev);
	if (!adapter)
		return -ENOMEM;
And reinvents err. So my point was to propagate it through.



>
>>
>> >+              goto unlock;
>> >+      }
>> >+      if (ret) {
>> >+              refcount_inc(&ret->refcount);
>> >+              goto unlock;
>> >+      }
>> >+      ret = no_free_ptr(a);
>> >+unlock:
>> >+      xa_unlock(&ice_adapters);
>> >+      return ret;
>> >+}
>> >+
>> >+void ice_adapter_put(const struct pci_dev *pdev)
>> >+{
>> >+      unsigned long index = ice_adapter_index(pdev);
>> >+      struct ice_adapter *a;
>> >+
>> >+      xa_lock(&ice_adapters);
>> >+      a = xa_load(&ice_adapters, index);
>> >+      if (WARN_ON(!a))
>> >+              goto unlock;
>> >+
>> >+      if (!refcount_dec_and_test(&a->refcount))
>> >+              goto unlock;
>> >+
>> >+      WARN_ON(__xa_erase(&ice_adapters, index) != a);
>>
>> Nice paranoia level :)
>>
>>
>> >+      ice_adapter_free(a);
>> >+unlock:
>> >+      xa_unlock(&ice_adapters);
>> >+}
>> >diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
>> >new file mode 100644
>> >index 000000000000..cb5a02eb24c1
>> >--- /dev/null
>> >+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
>> >@@ -0,0 +1,22 @@
>> >+/* SPDX-License-Identifier: GPL-2.0-only */
>> >+/* SPDX-FileCopyrightText: Copyright Red Hat */
>> >+
>> >+#ifndef _ICE_ADAPTER_H_
>> >+#define _ICE_ADAPTER_H_
>> >+
>> >+#include <linux/refcount_types.h>
>> >+
>> >+struct pci_dev;
>> >+
>> >+/**
>> >+ * struct ice_adapter - PCI adapter resources shared across PFs
>> >+ * @refcount: Reference count. struct ice_pf objects hold the references.
>> >+ */
>> >+struct ice_adapter {
>> >+      refcount_t refcount;
>> >+};
>> >+
>> >+struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev);
>> >+void ice_adapter_put(const struct pci_dev *pdev);
>> >+
>> >+#endif /* _ICE_ADAPTER_H */
>> >diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> >index 8f73ba77e835..413219d81a12 100644
>> >--- a/drivers/net/ethernet/intel/ice/ice_main.c
>> >+++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> >@@ -5093,6 +5093,7 @@ static int
>> > ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>> > {
>> >       struct device *dev = &pdev->dev;
>> >+      struct ice_adapter *adapter;
>> >       struct ice_pf *pf;
>> >       struct ice_hw *hw;
>> >       int err;
>> >@@ -5145,7 +5146,12 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>> >
>> >       pci_set_master(pdev);
>> >
>> >+      adapter = ice_adapter_get(pdev);
>> >+      if (!adapter)
>> >+              return -ENOMEM;
>> >+
>> >       pf->pdev = pdev;
>> >+      pf->adapter = adapter;
>> >       pci_set_drvdata(pdev, pf);
>> >       set_bit(ICE_DOWN, pf->state);
>> >       /* Disable service task until DOWN bit is cleared */
>> >@@ -5196,6 +5202,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>> > err_load:
>> >       ice_deinit(pf);
>> > err_init:
>> >+      ice_adapter_put(pdev);
>> >       pci_disable_device(pdev);
>> >       return err;
>> > }
>> >@@ -5302,6 +5309,7 @@ static void ice_remove(struct pci_dev *pdev)
>> >       ice_setup_mc_magic_wake(pf);
>> >       ice_set_wake(pf);
>> >
>> >+      ice_adapter_put(pdev);
>> >       pci_disable_device(pdev);
>> > }
>> >
>> >--
>> >2.43.2
>> >
>>
>