| 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: <ZCMSzzw1MTIc9dZW@nimitz>
Date: Tue, 28 Mar 2023 18:16:15 +0200
From: Piotr Raczynski <piotr.raczynski@...el.com>
To: Simon Horman <simon.horman@...igine.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
<michal.swiatkowski@...el.com>, <shiraz.saleem@...el.com>,
<jacob.e.keller@...el.com>, <sridhar.samudrala@...el.com>,
<jesse.brandeburg@...el.com>, <aleksander.lobakin@...el.com>,
<lukasz.czapnik@...el.com>,
Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
Subject: Re: [PATCH net-next v3 6/8] ice: add individual interrupt allocation
On Sun, Mar 26, 2023 at 03:18:03PM +0200, Simon Horman wrote:
> On Thu, Mar 23, 2023 at 01:24:38PM +0100, Piotr Raczynski wrote:
> > Currently interrupt allocations, depending on a feature are distributed
> > in batches. Also, after allocation there is a series of operations that
> > distributes per irq settings through that batch of interrupts.
> >
> > Although driver does not yet support dynamic interrupt allocation, keep
> > allocated interrupts in a pool and add allocation abstraction logic to
> > make code more flexible. Keep per interrupt information in the
> > ice_q_vector structure, which yields ice_vsi::base_vector redundant.
> > Also, as a result there are a few functions that can be removed.
> >
> > Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
> > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> > Signed-off-by: Piotr Raczynski <piotr.raczynski@...el.com>
>
> Reviewed-by: Simon Horman <simon.horman@...igine.com>
>
> I've made a few minor observations inline.
> I don't think there is a need to respin for any of them.
>
Thanks.
> > ---
> > drivers/net/ethernet/intel/ice/ice.h | 11 +-
> > drivers/net/ethernet/intel/ice/ice_arfs.c | 5 +-
> > drivers/net/ethernet/intel/ice/ice_base.c | 36 ++-
> > drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +-
> > drivers/net/ethernet/intel/ice/ice_idc.c | 45 ++--
> > drivers/net/ethernet/intel/ice/ice_irq.c | 46 +++-
> > drivers/net/ethernet/intel/ice/ice_irq.h | 3 +
> > drivers/net/ethernet/intel/ice/ice_lib.c | 225 ++-----------------
>
> Nice code removal from ice_lib.c :)
>
> > drivers/net/ethernet/intel/ice/ice_lib.h | 4 +-
> > drivers/net/ethernet/intel/ice/ice_main.c | 44 ++--
> > drivers/net/ethernet/intel/ice/ice_ptp.c | 2 +-
> > drivers/net/ethernet/intel/ice/ice_sriov.c | 2 +-
> > drivers/net/ethernet/intel/ice/ice_xsk.c | 5 +-
> > 13 files changed, 154 insertions(+), 276 deletions(-)
>
> ...
>
> > diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
> > index 1911d644dfa8..e5db23eaa3f4 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_base.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_base.c
> > @@ -118,9 +118,31 @@ static int ice_vsi_alloc_q_vector(struct ice_vsi *vsi, u16 v_idx)
> > q_vector->rx.itr_mode = ITR_DYNAMIC;
> > q_vector->tx.type = ICE_TX_CONTAINER;
> > q_vector->rx.type = ICE_RX_CONTAINER;
> > + q_vector->irq.index = -ENOENT;
> >
> > - if (vsi->type == ICE_VSI_VF)
> > + if (vsi->type == ICE_VSI_VF) {
> > + q_vector->reg_idx = ice_calc_vf_reg_idx(vsi->vf, q_vector);
> > goto out;
> > + } else if (vsi->type == ICE_VSI_CTRL && vsi->vf) {
> > + struct ice_vsi *ctrl_vsi = ice_get_vf_ctrl_vsi(pf, vsi);
> > +
> > + if (ctrl_vsi) {
> > + if (unlikely(!ctrl_vsi->q_vectors))
> > + return -ENOENT;
> > + q_vector->irq = ctrl_vsi->q_vectors[0]->irq;
> > + goto skip_alloc;
>
> nit: I think goto for error paths is very much the norm.
> But, FWIIW, I would have avoided using goto here.
Thanks will take a look.
>
> > + }
> > + }
> > +
> > + q_vector->irq = ice_alloc_irq(pf);
> > + if (q_vector->irq.index < 0) {
> > + kfree(q_vector);
> > + return -ENOMEM;
> > + }
> > +
> > +skip_alloc:
> > + q_vector->reg_idx = q_vector->irq.index;
> > +
> > /* only set affinity_mask if the CPU is online */
> > if (cpu_online(v_idx))
> > cpumask_set_cpu(v_idx, &q_vector->affinity_mask);
> > @@ -168,6 +190,18 @@ static void ice_free_q_vector(struct ice_vsi *vsi, int v_idx)
> > if (vsi->netdev)
> > netif_napi_del(&q_vector->napi);
> >
> > + /* release MSIX interrupt if q_vector had interrupt allocated */
> > + if (q_vector->irq.index < 0)
> > + goto free_q_vector;
> > +
> > + /* only free last VF ctrl vsi interrupt */
> > + if (vsi->type == ICE_VSI_CTRL && vsi->vf &&
> > + ice_get_vf_ctrl_vsi(pf, vsi))
> > + goto free_q_vector;
>
> Ditto (x2).
Will also take a look.
>
> > +
> > + ice_free_irq(pf, q_vector->irq);
> > +
> > +free_q_vector:
> > devm_kfree(dev, q_vector);
> > vsi->q_vectors[v_idx] = NULL;
> > }
>
> ...
>
> > diff --git a/drivers/net/ethernet/intel/ice/ice_irq.c b/drivers/net/ethernet/intel/ice/ice_irq.c
> > index f61be5d76373..ca1a1de26766 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_irq.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_irq.c
> > @@ -194,9 +194,53 @@ int ice_init_interrupt_scheme(struct ice_pf *pf)
> > }
> >
> > /* populate SW interrupts pool with number of OS granted IRQs. */
> > - pf->num_avail_sw_msix = (u16)vectors;
> > pf->irq_tracker->num_entries = (u16)vectors;
> > pf->irq_tracker->end = pf->irq_tracker->num_entries;
> >
> > return 0;
> > }
> > +
> > +/**
> > + * ice_alloc_irq - Allocate new interrupt vector
> > + * @pf: board private structure
> > + *
> > + * Allocate new interrupt vector for a given owner id.
> > + * return struct msi_map with interrupt details and track
> > + * allocated interrupt appropriately.
> > + *
> > + * This function mimics individual interrupt allocation,
> > + * even interrupts are actually already allocated with
> > + * pci_alloc_irq_vectors. Individual allocation helps
> > + * to track interrupts and simplifies interrupt related
> > + * handling.
> > + *
> > + * On failure, return map with negative .index. The caller
> > + * is expected to check returned map index.
> > + *
> > + */
> > +struct msi_map ice_alloc_irq(struct ice_pf *pf)
> > +{
> > + struct msi_map map = { .index = -ENOENT };
> > + int entry;
> > +
> > + entry = ice_get_res(pf, pf->irq_tracker);
> > + if (entry < 0)
>
> nit: map.index could be initialised here.
>
> > + return map;
> > +
> > + map.index = entry;
> > + map.virq = pci_irq_vector(pf->pdev, map.index);
> > +
> > + return map;
> > +}
Powered by blists - more mailing lists