Re: [PATCH 2/3 net-next v5] igb: refactor XDP registration

[Corinna Vinschen <vinschen@redhat.com>]

On Jan 18 16:05, Alexander Lobakin wrote:
> From: Corinna Vinschen <vinschen@redhat.com>
> Date: Mon, 17 Jan 2022 19:29:14 +0100
> 
> > On changing the RX ring parameters igb uses a hack to avoid a warning
> > when calling xdp_rxq_info_reg via igb_setup_rx_resources.  It just
> > clears the struct xdp_rxq_info content.
> > 
> > Change this to unregister if we're already registered instead.  Align
> > code to the igc code.
> > 
> > Fixes: 9cbc948b5a20c ("igb: add XDP support")
> > Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> > ---
> >  drivers/net/ethernet/intel/igb/igb_ethtool.c |  4 ----
> >  drivers/net/ethernet/intel/igb/igb_main.c    | 12 +++++++++---
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > index 51a2dcaf553d..2a5782063f4c 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > @@ -965,10 +965,6 @@ static int igb_set_ringparam(struct net_device *netdev,
> >  			memcpy(&temp_ring[i], adapter->rx_ring[i],
> >  			       sizeof(struct igb_ring));
> >  
> > -			/* Clear copied XDP RX-queue info */
> > -			memset(&temp_ring[i].xdp_rxq, 0,
> > -			       sizeof(temp_ring[i].xdp_rxq));
> > -
> >  			temp_ring[i].count = new_rx_count;
> >  			err = igb_setup_rx_resources(&temp_ring[i]);
> >  			if (err) {
> > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> > index 38ba92022cd4..cea89d301bfd 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > @@ -4352,7 +4352,7 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring)
> >  {
> >  	struct igb_adapter *adapter = netdev_priv(rx_ring->netdev);
> >  	struct device *dev = rx_ring->dev;
> > -	int size;
> > +	int size, res;
> >  
> >  	size = sizeof(struct igb_rx_buffer) * rx_ring->count;
> >  
> > @@ -4376,9 +4376,15 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring)
> >  	rx_ring->xdp_prog = adapter->xdp_prog;
> >  
> >  	/* XDP RX-queue info */
> > -	if (xdp_rxq_info_reg(&rx_ring->xdp_rxq, rx_ring->netdev,
> > -			     rx_ring->queue_index, 0) < 0)
> > +	if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
> > +		xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
> > +	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, rx_ring->netdev,
> > +			       rx_ring->queue_index, 0);
> > +	if (res < 0) {
> > +		dev_err(dev, "Failed to register xdp_rxq index %u\n",
> > +			rx_ring->queue_index);
> >  		goto err;
> 
> Error path always returns -ENOMEM, even in this case, and reports
> that it failed to allocate memory for rings. Handle this correctly
> and return `res` instead and without one more error message?

In that case, it makes sense to revert the code to the way igc did it,
rather then trying to do as igb did it.

I. e., for both drivers, call xdp_rxq_info_is_reg before the first
allocation took place, and just return immediately from there if it
fails.  Everything else complicates the code unnecessarily.

> As I mentioned a bit above, `res` is unused here as an error code,
> only to test the value on < 0. Does it make sense to add a new
> variable?

Following my above sugggestion, res will be used as error code, so
it should stay.

I'll provide a matching patchset later today.


Thanks,
Corinna