[Intel-wired-lan] [PATCH net] ice: Fix MSI-X vector fallback logic
Paul Menzel
pmenzel at molgen.mpg.de
Thu Jan 21 07:17:18 UTC 2021
Dear Brett,
Am 21.01.21 um 08:52 schrieb Brett Creeley:
> The current MSI-X enablement logic tries to enable best-case MSI-X
> vectors and if that fails we only support a bare-minimum set. This
> includes a single MSI-X for 1 Tx and 1 Rx queue and a single MSI-X
> for the OICR interrupt. Unfortunately, the driver fails to load when we
> don't get as many MSI-X as requested for a couple reasons.
>
> First, the code to allocate MSI-X in the driver tries to allocate
> num_online_cpus() MSI-X for LAN traffic without caring about the number
> of MSI-X actually enabled/requested from the kernel for LAN traffic.
> So, when calling ice_get_res() for the PF VSI, it returns failure
> because the number of available vectors is less than requested. Fix
> this by not allowing the PF VSI to allocation more than
> pf->num_lan_msix MSI-X vectors and pf->num_lan_msix Rx/Tx queues.
> Limiting the number of queues is done because we don't want more than
> 1 Tx/Rx queue per interrupt due to performance conerns.
>
> Second, the driver assigns pf->num_lan_msix = 2, to account for LAN
> traffic and the OICR. However, pf->num_lan_msix is only meant for LAN
> MSI-X. This is causing a failure when the PF VSI tries to
> allocate/reserve the minimum pf->num_lan_msix because the OICR MSI-X has
> already been reserved, so there may not be enough MSI-X vectors left.
> Fix this by setting pf->num_lan_msix = 1 for the failure case. Then the
> ICE_MIN_MSIX accounts for the LAN MSI-X and the OICR MSI-X needed for
> the failure case.
>
> Update the related defines used in ice_ena_msix_range() to align with
> the above behavior and remove the unused RDMA defines because RDMA is
> currently not supported. Also, remove the now incorrect comment.
>
> Also, prevent users from enabling more combined queues than there are
> MSI-X available via ethtool.
>
> Fixes: Commit 152b978a1f90 ("ice: Rework ice_ena_msix_range")
> Fixes: Commit 87324e747fde ("ice: Implement ethtool ops for channels")
The word *Commit* does not need to be put in there.
Could you split the ethtool change into a separate commit?
Also, can you document your test setup so the driver failed to load?
Kind regards,
Paul
> Signed-off-by: Brett Creeley <brett.creeley at intel.com>
> ---
> drivers/net/ethernet/intel/ice/ice.h | 4 +++-
> drivers/net/ethernet/intel/ice/ice_ethtool.c | 8 ++++----
> drivers/net/ethernet/intel/ice/ice_lib.c | 14 +++++++++-----
> drivers/net/ethernet/intel/ice/ice_main.c | 8 ++------
> 4 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 6efafe7d8a62..619d93f8b54c 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -68,7 +68,9 @@
> #define ICE_INT_NAME_STR_LEN (IFNAMSIZ + 16)
> #define ICE_AQ_LEN 64
> #define ICE_MBXSQ_LEN 64
> -#define ICE_MIN_MSIX 2
> +#define ICE_MIN_LAN_TXRX_MSIX 1
> +#define ICE_MIN_LAN_OICR_MSIX 1
> +#define ICE_MIN_MSIX (ICE_MIN_LAN_TXRX_MSIX + ICE_MIN_LAN_OICR_MSIX)
> #define ICE_FDIR_MSIX 1
> #define ICE_NO_VSI 0xffff
> #define ICE_VSI_MAP_CONTIG 0
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index 41427302332c..aebebd2102da 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -3265,8 +3265,8 @@ ice_set_rxfh(struct net_device *netdev, const u32 *indir, const u8 *key,
> */
> static int ice_get_max_txq(struct ice_pf *pf)
> {
> - return min_t(int, num_online_cpus(),
> - pf->hw.func_caps.common_cap.num_txq);
> + return min3(pf->num_lan_msix, (u16)num_online_cpus(),
> + (u16)pf->hw.func_caps.common_cap.num_txq);
> }
>
> /**
> @@ -3275,8 +3275,8 @@ static int ice_get_max_txq(struct ice_pf *pf)
> */
> static int ice_get_max_rxq(struct ice_pf *pf)
> {
> - return min_t(int, num_online_cpus(),
> - pf->hw.func_caps.common_cap.num_rxq);
> + return min3(pf->num_lan_msix, (u16)num_online_cpus(),
> + (u16)pf->hw.func_caps.common_cap.num_rxq);
> }
>
> /**
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 3df67486d42d..ad9c22a1b97a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -161,8 +161,9 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16 vf_id)
>
> switch (vsi->type) {
> case ICE_VSI_PF:
> - vsi->alloc_txq = min_t(int, ice_get_avail_txq_count(pf),
> - num_online_cpus());
> + vsi->alloc_txq = min3(pf->num_lan_msix,
> + ice_get_avail_txq_count(pf),
> + (u16)num_online_cpus());
> if (vsi->req_txq) {
> vsi->alloc_txq = vsi->req_txq;
> vsi->num_txq = vsi->req_txq;
> @@ -174,8 +175,9 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16 vf_id)
> if (!test_bit(ICE_FLAG_RSS_ENA, pf->flags)) {
> vsi->alloc_rxq = 1;
> } else {
> - vsi->alloc_rxq = min_t(int, ice_get_avail_rxq_count(pf),
> - num_online_cpus());
> + vsi->alloc_rxq = min3(pf->num_lan_msix,
> + ice_get_avail_rxq_count(pf),
> + (u16)num_online_cpus());
> if (vsi->req_rxq) {
> vsi->alloc_rxq = vsi->req_rxq;
> vsi->num_rxq = vsi->req_rxq;
> @@ -184,7 +186,9 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16 vf_id)
>
> pf->num_lan_rx = vsi->alloc_rxq;
>
> - vsi->num_q_vectors = max_t(int, vsi->alloc_rxq, vsi->alloc_txq);
> + vsi->num_q_vectors = min_t(int, pf->num_lan_msix,
> + max_t(int, vsi->alloc_rxq,
> + vsi->alloc_txq));
> break;
> case ICE_VSI_VF:
> vf = &pf->vf[vsi->vf_id];
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index fb81aa5979e3..e10ca8929f85 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -3430,18 +3430,14 @@ static int ice_ena_msix_range(struct ice_pf *pf)
> if (v_actual < v_budget) {
> dev_warn(dev, "not enough OS MSI-X vectors. requested = %d, obtained = %d\n",
> v_budget, v_actual);
> -/* 2 vectors each for LAN and RDMA (traffic + OICR), one for flow director */
> -#define ICE_MIN_LAN_VECS 2
> -#define ICE_MIN_RDMA_VECS 2
> -#define ICE_MIN_VECS (ICE_MIN_LAN_VECS + ICE_MIN_RDMA_VECS + 1)
>
> - if (v_actual < ICE_MIN_LAN_VECS) {
> + if (v_actual < ICE_MIN_MSIX) {
> /* error if we can't get minimum vectors */
> pci_disable_msix(pf->pdev);
> err = -ERANGE;
> goto msix_err;
> } else {
> - pf->num_lan_msix = ICE_MIN_LAN_VECS;
> + pf->num_lan_msix = ICE_MIN_LAN_TXRX_MSIX;
> }
> }
>
>
More information about the Intel-wired-lan
mailing list