[PATCH v12 11/12] net/tap: do not mark queue full as error

[Stephen Hemminger]

On Mon, 20 May 2024 18:52:27 +0100
Ferruh Yigit <ferruh.yigit at amd.com> wrote:

> On 5/2/2024 10:31 PM, Stephen Hemminger wrote:
> > If tap device in kernel returns EAGAIN that means it is full.
> > That is not an error.
> > 
> > Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
> > ---
> >  drivers/net/tap/rte_eth_tap.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> > index 2484a82ccb..485bd35912 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -542,7 +542,6 @@ tap_write_mbufs(struct tx_queue *txq, uint16_t num_mbufs,
> >  		struct rte_mbuf *seg = mbuf;
> >  		uint64_t l4_ol_flags;
> >  		int proto;
> > -		int n;
> >  		int j;
> >  		int k; /* current index in iovecs for copying segments */
> >  
> > @@ -647,14 +646,18 @@ tap_write_mbufs(struct tx_queue *txq, uint16_t num_mbufs,
> >  		}
> >  
> >  		/* copy the tx frame data */
> > -		n = writev(process_private->fds[txq->queue_id], iovecs, k);
> > -		if (n <= 0)
> > -			return -1;
> > +		if (unlikely(writev(process_private->fds[txq->queue_id], iovecs, k) < 0)) {
> > +			TAP_LOG(DEBUG, "writev (qid=%u fd=%d) %s",
> > +				txq->queue_id, process_private->fds[txq->queue_id],
> > +				strerror(errno));
> >  
> 
> Do we really want logging in datapath?
> 
> > +			return (errno == EAGAIN) ? 0 : -1;
> >  
> 
> Nack.
> 
> Returning '0' will cause 'pmd_tx_burst()' to continue and increase
> 'num_tx', this will mislead as packet sent successfully.
> 
> We should have three return values from 'tap_write_mbufs()':
> <0 -> Error, 'pmd_tx_burst()' should break, stats.errors updated.
>  0 ->  'pmd_tx_burst()' should break, valid num_tx returned
> >0 -> 'pmd_tx_burst()' work as it is.  
> 
> > +		}
> >  
> >  		(*num_packets)++;
> >  		(*num_tx_bytes) += rte_pktmbuf_pkt_len(mbuf);
> >  	}
> > -	return 0;
> > +
> > +	return 1;
> >   
> 
> Why '1', wouldn't it be better to return number of packets written
> successfully.
> 


Decided to drop this patch. Doing GSO inside the TAP device is the
wrong place to do it. Kernel already has API to do GSO and checksum
offload and it will be simpler and faster to use that. When TAP
device is updated not to do the GSO here, this code goes away
and handling the full condition properly gets much easier.

When TAP is doing GSO there are lots of corner cases where a expanded
GSO packet could get only partially sent.