| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DM4PR11MB6502C10E24DDC217422D8456D467A@DM4PR11MB6502.namprd11.prod.outlook.com>
Date: Wed, 28 May 2025 21:35:02 +0000
From: "Hay, Joshua A" <joshua.a.hay@...el.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>, "Nikolova, Tatyana E"
<tatyana.e.nikolova@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"jgg@...dia.com" <jgg@...dia.com>, "leon@...nel.org" <leon@...nel.org>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "kuba@...nel.org"
<kuba@...nel.org>, "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
Subject: RE: [Intel-wired-lan] [iwl-next 1/6] idpf: use reserved RDMA vectors
from control plane
> -----Original Message-----
> From: Paul Menzel <pmenzel@...gen.mpg.de>
> Sent: Saturday, May 24, 2025 10:24 PM
> To: Nikolova, Tatyana E <tatyana.e.nikolova@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; jgg@...dia.com; leon@...nel.org; linux-
> rdma@...r.kernel.org; netdev@...r.kernel.org; kuba@...nel.org; Nguyen,
> Anthony L <anthony.l.nguyen@...el.com>; Hay, Joshua A
> <joshua.a.hay@...el.com>
> Subject: Re: [Intel-wired-lan] [iwl-next 1/6] idpf: use reserved RDMA vectors
> from control plane
>
> Dear Tatyana,
>
>
> Thank you for your patch.
>
> Am 23.05.25 um 19:04 schrieb Tatyana Nikolova:
> > From: Joshua Hay <joshua.a.hay@...el.com>
> >
> > Fetch the number of reserved RDMA vectors from the control plane.
> > Adjust the number of reserved LAN vectors if necessary. Adjust the
> > minimum number of vectors the OS should reserve to include RDMA; and
> > fail if the OS cannot reserve enough vectors for the minimum number of
> > LAN and RDMA vectors required. Create a separate msix table for the
> > reserved RDMA vectors, which will just get handed off to the RDMA core
> > device to do with what it will.
>
> How can this all be tested? It’d be great if you added the commands and
> outputs.
This was tested on a range of hosts/platforms since the number of reserved RDMA vectors is based solely on the device configuration, which is not exposed to the end user. The user can confirm how many vectors the drivers have after everything is loaded properly though. From the RDMA driver perspective it will either have IDPF_MIN_RDMA_VEC vectors or whatever was provided by the device.
>
> > Reviewed-by: Madhu Chittim <madhu.chittim@...el.com>
> > Signed-off-by: Joshua Hay <joshua.a.hay@...el.com>
> > Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@...el.com>
> > ---
> > drivers/net/ethernet/intel/idpf/idpf.h | 28 +++++++-
> > drivers/net/ethernet/intel/idpf/idpf_lib.c | 74 +++++++++++++++++----
> > drivers/net/ethernet/intel/idpf/idpf_txrx.h | 1 +
> > drivers/net/ethernet/intel/idpf/virtchnl2.h | 5 +-
> > 4 files changed, 92 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf.h
> b/drivers/net/ethernet/intel/idpf/idpf.h
> > index 66544faab710..8ef7120e6717 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf.h
> > +++ b/drivers/net/ethernet/intel/idpf/idpf.h
> > @@ -489,10 +489,11 @@ struct idpf_vc_xn_manager;
> > * @flags: See enum idpf_flags
> > * @reset_reg: See struct idpf_reset_reg
> > * @hw: Device access data
> > - * @num_req_msix: Requested number of MSIX vectors
> > * @num_avail_msix: Available number of MSIX vectors
> > * @num_msix_entries: Number of entries in MSIX table
> > * @msix_entries: MSIX table
> > + * @num_rdma_msix_entries: Available number of MSIX vectors for RDMA
> > + * @rdma_msix_entries: RDMA MSIX table
> > * @req_vec_chunks: Requested vector chunk data
> > * @mb_vector: Mailbox vector data
> > * @vector_stack: Stack to store the msix vector indexes
> > @@ -542,10 +543,11 @@ struct idpf_adapter {
> > DECLARE_BITMAP(flags, IDPF_FLAGS_NBITS);
> > struct idpf_reset_reg reset_reg;
> > struct idpf_hw hw;
> > - u16 num_req_msix;
> > u16 num_avail_msix;
> > u16 num_msix_entries;
> > struct msix_entry *msix_entries;
> > + u16 num_rdma_msix_entries;
> > + struct msix_entry *rdma_msix_entries;
> > struct virtchnl2_alloc_vectors *req_vec_chunks;
> > struct idpf_q_vector mb_vector;
> > struct idpf_vector_lifo vector_stack;
> > @@ -609,6 +611,17 @@ static inline int idpf_is_queue_model_split(u16
> q_model)
> > bool idpf_is_capability_ena(struct idpf_adapter *adapter, bool all,
> > enum idpf_cap_field field, u64 flag);
> >
> > +/**
> > + * idpf_is_rdma_cap_ena - Determine if RDMA is supported
> > + * @adapter: private data struct
> > + *
> > + * Return: true if RDMA capability is enabled, false otherwise
> > + */
> > +static inline bool idpf_is_rdma_cap_ena(struct idpf_adapter *adapter)
> > +{
> > + return idpf_is_cap_ena(adapter, IDPF_OTHER_CAPS,
> VIRTCHNL2_CAP_RDMA);
> > +}
> > +
> > #define IDPF_CAP_RSS (\
> > VIRTCHNL2_CAP_RSS_IPV4_TCP |\
> > VIRTCHNL2_CAP_RSS_IPV4_TCP |\
> > @@ -663,6 +676,17 @@ static inline u16 idpf_get_reserved_vecs(struct
> idpf_adapter *adapter)
> > return le16_to_cpu(adapter->caps.num_allocated_vectors);
> > }
> >
> > +/**
> > + * idpf_get_reserved_rdma_vecs - Get reserved RDMA vectors
> > + * @adapter: private data struct
> > + *
> > + * Return: number of vectors reserved for RDMA
> > + */
> > +static inline u16 idpf_get_reserved_rdma_vecs(struct idpf_adapter
> *adapter)
> > +{
> > + return le16_to_cpu(adapter->caps.num_rdma_allocated_vectors);
> > +}
> > +
> > /**
> > * idpf_get_default_vports - Get default number of vports
> > * @adapter: private data struct
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > index aa755dedb41d..0d5c57502cac 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > @@ -87,6 +87,8 @@ void idpf_intr_rel(struct idpf_adapter *adapter)
> > idpf_deinit_vector_stack(adapter);
> > kfree(adapter->msix_entries);
> > adapter->msix_entries = NULL;
> > + kfree(adapter->rdma_msix_entries);
> > + adapter->rdma_msix_entries = NULL;
> > }
> >
> > /**
> > @@ -314,13 +316,33 @@ int idpf_req_rel_vector_indexes(struct
> idpf_adapter *adapter,
> > */
> > int idpf_intr_req(struct idpf_adapter *adapter)
> > {
> > + u16 num_lan_vecs, min_lan_vecs, num_rdma_vecs = 0,
> min_rdma_vecs = 0;
> > u16 default_vports = idpf_get_default_vports(adapter);
> > int num_q_vecs, total_vecs, num_vec_ids;
> > int min_vectors, v_actual, err;
>
> Unrelated, but `v_actual` is strangely named, when all other variables
> seem to use vec.
Good point, I can change it in v2 if that is the ask.
>
> > unsigned int vector;
> > u16 *vecids;
> > + int i;
>
> size_t?
Sorry, can you please explain why size_t would be preferred here?
>
> >
> > total_vecs = idpf_get_reserved_vecs(adapter);
> > + num_lan_vecs = total_vecs;
> > + if (idpf_is_rdma_cap_ena(adapter)) {
> > + num_rdma_vecs = idpf_get_reserved_rdma_vecs(adapter);
> > + min_rdma_vecs = IDPF_MIN_RDMA_VEC;
> > +
> > + if (!num_rdma_vecs) {
> > + /* If idpf_get_reserved_rdma_vecs is 0, vectors are
> > + * pulled from the LAN pool.
> > + */
> > + num_rdma_vecs = min_rdma_vecs;
> > + } else if (num_rdma_vecs < min_rdma_vecs) {
> > + dev_err(&adapter->pdev->dev,
> > + "Not enough vectors reserved for RDMA (min:
> %u, current: %u)\n",
> > + min_rdma_vecs, num_rdma_vecs);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > num_q_vecs = total_vecs - IDPF_MBX_Q_VEC;
> >
> > err = idpf_send_alloc_vectors_msg(adapter, num_q_vecs);
> > @@ -331,52 +353,75 @@ int idpf_intr_req(struct idpf_adapter *adapter)
> > return -EAGAIN;
> > }
> >
> > - min_vectors = IDPF_MBX_Q_VEC + IDPF_MIN_Q_VEC *
> default_vports;
> > + min_lan_vecs = IDPF_MBX_Q_VEC + IDPF_MIN_Q_VEC *
> default_vports;
> > + min_vectors = min_lan_vecs + min_rdma_vecs;
> > v_actual = pci_alloc_irq_vectors(adapter->pdev, min_vectors,
> > total_vecs, PCI_IRQ_MSIX);
> > if (v_actual < min_vectors) {
> > - dev_err(&adapter->pdev->dev, "Failed to allocate MSIX
> vectors: %d\n",
> > + dev_err(&adapter->pdev->dev, "Failed to allocate minimum
> MSIX vectors required: %d\n",
> > v_actual);
> > err = -EAGAIN;
> > goto send_dealloc_vecs;
> > }
> >
> > - adapter->msix_entries = kcalloc(v_actual, sizeof(struct msix_entry),
> > - GFP_KERNEL);
> > + if (idpf_is_rdma_cap_ena(adapter)) {
> > + if (v_actual < total_vecs) {
> > + dev_warn(&adapter->pdev->dev,
> > + "Warning: not enough vectors available.
> Defaulting to minimum for RDMA and remaining for LAN.\n");
>
> Also log `v_actual`, `total_vecs` and `IDPF_MIN_RDMA_VEC`?
Will add in v2.
>
> > + num_rdma_vecs = IDPF_MIN_RDMA_VEC;
> > + }
> >
> > + adapter->rdma_msix_entries =
> > + kcalloc(num_rdma_vecs,
> > + sizeof(struct msix_entry), GFP_KERNEL);
> > + if (!adapter->rdma_msix_entries) {
> > + err = -ENOMEM;
> > + goto free_irq;
> > + }
> > + }
> > +
> > + num_lan_vecs = v_actual - num_rdma_vecs;
> > + adapter->msix_entries = kcalloc(num_lan_vecs, sizeof(struct
> msix_entry),
> > + GFP_KERNEL);
> > if (!adapter->msix_entries) {
> > err = -ENOMEM;
> > - goto free_irq;
> > + goto free_rdma_msix;
> > }
> >
> > idpf_set_mb_vec_id(adapter);
> >
> > - vecids = kcalloc(total_vecs, sizeof(u16), GFP_KERNEL);
> > + vecids = kcalloc(v_actual, sizeof(u16), GFP_KERNEL);
> > if (!vecids) {
> > err = -ENOMEM;
> > goto free_msix;
> > }
> >
> > - num_vec_ids = idpf_get_vec_ids(adapter, vecids, total_vecs,
> > + num_vec_ids = idpf_get_vec_ids(adapter, vecids, v_actual,
> > &adapter->req_vec_chunks->vchunks);
> > if (num_vec_ids < v_actual) {
> > err = -EINVAL;
> > goto free_vecids;
> > }
> >
> > - for (vector = 0; vector < v_actual; vector++) {
> > - adapter->msix_entries[vector].entry = vecids[vector];
> > - adapter->msix_entries[vector].vector =
> > + for (i = 0, vector = 0; vector < num_lan_vecs; vector++, i++) {
> > + adapter->msix_entries[i].entry = vecids[vector];
> > + adapter->msix_entries[i].vector =
> > + pci_irq_vector(adapter->pdev, vector);
>
> Excuse my ignorance, but why are two counting variables needed, that
> seem to be identical?
Ah, good catch, will just use vector above.
>
> > + }
> > + for (i = 0; i < num_rdma_vecs; vector++, i++) {
> > + adapter->rdma_msix_entries[i].entry = vecids[vector];
> > + adapter->rdma_msix_entries[i].vector =
> > pci_irq_vector(adapter->pdev, vector);
> > }
> >
> > - adapter->num_req_msix = total_vecs;
> > - adapter->num_msix_entries = v_actual;
> > /* 'num_avail_msix' is used to distribute excess vectors to the vports
> > * after considering the minimum vectors required per each default
> > * vport
> > */
> > - adapter->num_avail_msix = v_actual - min_vectors;
> > + adapter->num_avail_msix = num_lan_vecs - min_lan_vecs;
> > + adapter->num_msix_entries = num_lan_vecs;
> > + if (idpf_is_rdma_cap_ena(adapter))
> > + adapter->num_rdma_msix_entries = num_rdma_vecs;
> >
> > /* Fill MSIX vector lifo stack with vector indexes */
> > err = idpf_init_vector_stack(adapter);
> > @@ -398,6 +443,9 @@ int idpf_intr_req(struct idpf_adapter *adapter)
> > free_msix:
> > kfree(adapter->msix_entries);
> > adapter->msix_entries = NULL;
> > +free_rdma_msix:
> > + kfree(adapter->rdma_msix_entries);
> > + adapter->rdma_msix_entries = NULL;
> > free_irq:
> > pci_free_irq_vectors(adapter->pdev);
> > send_dealloc_vecs:
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> > index b029f566e57c..9cb97397d89b 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> > @@ -57,6 +57,7 @@
> > /* Default vector sharing */
> > #define IDPF_MBX_Q_VEC 1
> > #define IDPF_MIN_Q_VEC 1
> > +#define IDPF_MIN_RDMA_VEC 2
> >
> > #define IDPF_DFLT_TX_Q_DESC_COUNT 512
> > #define IDPF_DFLT_TX_COMPLQ_DESC_COUNT 512
> > diff --git a/drivers/net/ethernet/intel/idpf/virtchnl2.h
> b/drivers/net/ethernet/intel/idpf/virtchnl2.h
> > index 63deb120359c..80c17e4a394e 100644
> > --- a/drivers/net/ethernet/intel/idpf/virtchnl2.h
> > +++ b/drivers/net/ethernet/intel/idpf/virtchnl2.h
> > @@ -473,6 +473,8 @@ VIRTCHNL2_CHECK_STRUCT_LEN(8,
> virtchnl2_version_info);
> > * segment offload.
> > * @max_hdr_buf_per_lso: Max number of header buffers that can be used
> for
> > * an LSO.
> > + * @num_rdma_allocated_vectors: Maximum number of allocated RDMA
> vectors for
> > + * the device.
> > * @pad1: Padding for future extensions.
> > *
> > * Dataplane driver sends this message to CP to negotiate capabilities and
> > @@ -520,7 +522,8 @@ struct virtchnl2_get_capabilities {
> > __le32 device_type;
> > u8 min_sso_packet_len;
> > u8 max_hdr_buf_per_lso;
> > - u8 pad1[10];
> > + __le16 num_rdma_allocated_vectors;
> > + u8 pad1[8];
> > };
> > VIRTCHNL2_CHECK_STRUCT_LEN(80, virtchnl2_get_capabilities);
>
>
> Kind regards,
>
> Paul
Thank you!
Josh
Powered by blists - more mailing lists