Re: [PATCH net-next v2 2/6] tsnep: Add XDP TX support

[Gerhard Engleder <gerhard@xxxxxxxxxxxxxxxxxxxxx>]

On 08.12.22 15:10, Maciej Fijalkowski wrote:
> > @@ -65,7 +71,11 @@ struct tsnep_tx_entry {
> >   
> >   	u32 properties;
> >   
> > -	struct sk_buff *skb;
> > +	enum tsnep_tx_type type;
> > +	union {
> > +		struct sk_buff *skb;
> > +		struct xdp_frame *xdpf;
> > +	};
> >   	size_t len;
> >   	DEFINE_DMA_UNMAP_ADDR(dma);
> >   };
> > diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
> > index a28fde9fb060..b97cfd5fa1fa 100644
> > --- a/drivers/net/ethernet/engleder/tsnep_main.c
> > +++ b/drivers/net/ethernet/engleder/tsnep_main.c
> > @@ -310,10 +310,11 @@ static void tsnep_tx_activate(struct tsnep_tx *tx, int index, int length,
> >   	struct tsnep_tx_entry *entry = &tx->entry[index];
> >   
> >   	entry->properties = 0;
> > -	if (entry->skb) {
> > +	if (entry->skb || entry->xdpf) {
>
> i think this change is redundant, you could keep a single check as skb and
> xdpf ptrs share the same memory, but i guess this makes it more obvious

Yes it is actually redundant. I thought it is not a good idea to rely on
the union in the code.

> > +/* This function requires __netif_tx_lock is held by the caller. */
> > +static int tsnep_xdp_xmit_frame_ring(struct xdp_frame *xdpf,
> > +				     struct tsnep_tx *tx, bool dma_map)
> > +{
> > +	struct skb_shared_info *shinfo = xdp_get_shared_info_from_frame(xdpf);
> > +	unsigned long flags;
> > +	int count = 1;
> > +	struct tsnep_tx_entry *entry;
> > +	int length;
> > +	int i;
> > +	int retval;
> > +
> > +	if (unlikely(xdp_frame_has_frags(xdpf)))
> > +		count += shinfo->nr_frags;
> > +
> > +	spin_lock_irqsave(&tx->lock, flags);
> > +
> > +	if (tsnep_tx_desc_available(tx) < (MAX_SKB_FRAGS + 1 + count)) {
>
> Wouldn't count + 1 be sufficient to check against the descs available?
> if there are frags then you have already accounted them under count
> variable so i feel like MAX_SKB_FRAGS is redundant.

In the standard TX path tsnep_xmit_frame_ring() would stop the queue if
less than MAX_SKB_FRAGS + 1 descriptors are available. I wanted to keep
that stop queue logic in tsnep_xmit_frame_ring() by ensuring that XDP
never exceeds this limit (similar to STMMAC_TX_THRESH of stmmac).

So this line checks if enough descriptors are available and that the
queue would not have been stopped by tsnep_xmit_frame_ring().

I could improve the comment below.

> > +		/* prevent full TX ring due to XDP */
> > +		spin_unlock_irqrestore(&tx->lock, flags);
> > +
> > +		return -EBUSY;
> > +	}

Gerhard