On Mon, 20 May 2024 18:52:27 +0100
Ferruh Yigit <ferruh.yi...@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 <step...@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.
