netdev - Re: [PATCH v2 net-next 3/5] ionic: use per-queue xdp

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zs2/N7K/IIATcANm@lzaremba-mobl.ger.corp.intel.com>
Date: Tue, 27 Aug 2024 13:57:43 +0200
From: Larysa Zaremba <larysa.zaremba@...el.com>
To: Brett Creeley <brett.creeley@....com>
CC: <netdev@...r.kernel.org>, <davem@...emloft.net>, <kuba@...nel.org>,
	<edumazet@...gle.com>, <pabeni@...hat.com>, <shannon.nelson@....com>
Subject: Re: [PATCH v2 net-next 3/5] ionic: use per-queue xdp_prog

On Tue, Aug 27, 2024 at 01:44:10PM +0200, Larysa Zaremba wrote:
> On Mon, Aug 26, 2024 at 11:44:20AM -0700, Brett Creeley wrote:
> > From: Shannon Nelson <shannon.nelson@....com>
> > 
> > We originally were using a per-interface xdp_prog variable to track
> > a loaded XDP program since we knew there would never be support for a
> > per-queue XDP program.  With that, we only built the per queue rxq_info
> > struct when an XDP program was loaded and removed it on XDP program unload,
> > and used the pointer as an indicator in the Rx hotpath to know to how build
> > the buffers.  However, that's really not the model generally used, and
> > makes a conversion to page_pool Rx buffer cacheing a little problematic.
> > 
> > This patch converts the driver to use the more common approach of using
> > a per-queue xdp_prog pointer to work out buffer allocations and need
> > for bpf_prog_run_xdp().  We jostle a couple of fields in the queue struct
> > in order to keep the new xdp_prog pointer in a warm cacheline.
> > 
> > Signed-off-by: Shannon Nelson <shannon.nelson@....com>
> > Signed-off-by: Brett Creeley <brett.creeley@....com>
> 
> Reviewed-by: Larysa Zaremba <larysa.zaremba@...el.com>
>

I would like to rewoke the tag, see below why.

> If you happen to send another version, please include in a commit message a note 
> about READ_ONCE() removal. The removal itself is OK, but an indication that this 
> was intentional would be nice.
> 
> > ---
> >  .../net/ethernet/pensando/ionic/ionic_dev.h   |  7 +++++--
> >  .../net/ethernet/pensando/ionic/ionic_lif.c   | 14 +++++++------
> >  .../net/ethernet/pensando/ionic/ionic_txrx.c  | 21 +++++++++----------
> >  3 files changed, 23 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
> > index c647033f3ad2..19ae68a86a0b 100644
> > --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
> > +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
> > @@ -238,9 +238,8 @@ struct ionic_queue {
> >  	unsigned int index;
> >  	unsigned int num_descs;
> >  	unsigned int max_sg_elems;
> > +
> >  	u64 features;
> > -	unsigned int type;
> > -	unsigned int hw_index;
> >  	unsigned int hw_type;
> >  	bool xdp_flush;
> >  	union {
> > @@ -261,7 +260,11 @@ struct ionic_queue {
> >  		struct ionic_rxq_sg_desc *rxq_sgl;
> >  	};
> >  	struct xdp_rxq_info *xdp_rxq_info;
> > +	struct bpf_prog *xdp_prog;
> >  	struct ionic_queue *partner;
> > +
> > +	unsigned int type;
> > +	unsigned int hw_index;
> >  	dma_addr_t base_pa;
> >  	dma_addr_t cmb_base_pa;
> >  	dma_addr_t sg_base_pa;
> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> > index aa0cc31dfe6e..0fba2df33915 100644
> > --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> > +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> > @@ -2700,24 +2700,24 @@ static int ionic_xdp_register_rxq_info(struct ionic_queue *q, unsigned int napi_
> >  
> >  static int ionic_xdp_queues_config(struct ionic_lif *lif)
> >  {
> > +	struct bpf_prog *xdp_prog;
> >  	unsigned int i;
> >  	int err;
> >  
> >  	if (!lif->rxqcqs)
> >  		return 0;
> >  
> > -	/* There's no need to rework memory if not going to/from NULL program.
> > -	 * If there is no lif->xdp_prog, there should also be no q.xdp_rxq_info
> > -	 * This way we don't need to keep an *xdp_prog in every queue struct.
> > -	 */
> > -	if (!lif->xdp_prog == !lif->rxqcqs[0]->q.xdp_rxq_info)
> > +	/* There's no need to rework memory if not going to/from NULL program.  */
> > +	xdp_prog = READ_ONCE(lif->xdp_prog);
> > +	if (!xdp_prog == !lif->rxqcqs[0]->q.xdp_prog)
> >  		return 0;

In a case when we replace a non-NULL program with another non-NULL program this 
would create a situation where lif and queues have different pointers on them.

> >  
> >  	for (i = 0; i < lif->ionic->nrxqs_per_lif && lif->rxqcqs[i]; i++) {
> >  		struct ionic_queue *q = &lif->rxqcqs[i]->q;
> >  
> > -		if (q->xdp_rxq_info) {
> > +		if (q->xdp_prog) {
> >  			ionic_xdp_unregister_rxq_info(q);
> > +			q->xdp_prog = NULL;
> >  			continue;
> >  		}
> >  
> > @@ -2727,6 +2727,7 @@ static int ionic_xdp_queues_config(struct ionic_lif *lif)
> >  				i, err);
> >  			goto err_out;
> >  		}
> > +		q->xdp_prog = xdp_prog;
> >  	}
> >  
> >  	return 0;

[...]