Re: [PATCH net-next v3 6/6] tsnep: Add XDP socket zero-copy TX support

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Fri, Apr 21, 2023 at 09:02:56PM +0200, Gerhard Engleder wrote:
> On 20.04.23 22:17, Maciej Fijalkowski wrote:
> > On Tue, Apr 18, 2023 at 09:04:59PM +0200, Gerhard Engleder wrote:
> > > Send and complete XSK pool frames within TX NAPI context. NAPI context
> > > is triggered by ndo_xsk_wakeup.
> > > 
> > > Test results with A53 1.2GHz:
> > > 
> > > xdpsock txonly copy mode:
> > >                     pps            pkts           1.00
> > > tx                 284,409        11,398,144
> > > Two CPUs with 100% and 10% utilization.
> > > 
> > > xdpsock txonly zero-copy mode:
> > >                     pps            pkts           1.00
> > > tx                 511,929        5,890,368
> > > Two CPUs with 100% and 1% utilization.
> > 
> > Hmm, I think l2fwd ZC numbers should be included here not in the previous
> > patch?
> 
> Will be done.
> 
> > > 
> > > Packet rate increases and CPU utilization is reduced.
> > > 
> > > Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> > > ---
> > >   drivers/net/ethernet/engleder/tsnep.h      |   2 +
> > >   drivers/net/ethernet/engleder/tsnep_main.c | 127 +++++++++++++++++++--
> > >   2 files changed, 119 insertions(+), 10 deletions(-)
> > > 
> 
> (...)
> 
> > >   static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
> > >   {
> > >   	struct tsnep_tx_entry *entry;
> > >   	struct netdev_queue *nq;
> > > +	int xsk_frames = 0;
> > >   	int budget = 128;
> > >   	int length;
> > >   	int count;
> > > @@ -676,7 +771,7 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
> > >   		if ((entry->type & TSNEP_TX_TYPE_SKB) &&
> > >   		    skb_shinfo(entry->skb)->nr_frags > 0)
> > >   			count += skb_shinfo(entry->skb)->nr_frags;
> > > -		else if (!(entry->type & TSNEP_TX_TYPE_SKB) &&
> > > +		else if ((entry->type & TSNEP_TX_TYPE_XDP) &&
> > >   			 xdp_frame_has_frags(entry->xdpf))
> > >   			count += xdp_get_shared_info_from_frame(entry->xdpf)->nr_frags;
> > > @@ -705,9 +800,11 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
> > >   		if (entry->type & TSNEP_TX_TYPE_SKB)
> > >   			napi_consume_skb(entry->skb, napi_budget);
> > > -		else
> > > +		else if (entry->type & TSNEP_TX_TYPE_XDP)
> > >   			xdp_return_frame_rx_napi(entry->xdpf);
> > > -		/* xdpf is union with skb */
> > > +		else
> > > +			xsk_frames++;
> > > +		/* xdpf and zc are union with skb */
> > >   		entry->skb = NULL;
> > >   		tx->read = (tx->read + count) & TSNEP_RING_MASK;
> > > @@ -718,6 +815,14 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
> > >   		budget--;
> > >   	} while (likely(budget));
> > > +	if (tx->xsk_pool) {
> > > +		if (xsk_frames)
> > > +			xsk_tx_completed(tx->xsk_pool, xsk_frames);
> > > +		if (xsk_uses_need_wakeup(tx->xsk_pool))
> > > +			xsk_set_tx_need_wakeup(tx->xsk_pool);
> > > +		tsnep_xdp_xmit_zc(tx);
> > 
> > would be good to signal to NAPI if we are done with the work or is there a
> > need to be rescheduled (when you didn't manage to consume all of the descs
> > from XSK Tx ring).
> 
> In my opinion this is already done. If some budget is left, then we are
> done and tsnep_tx_poll() returns true to signal work is complete. If
> buget gets zero, then tsnep_tx_poll() returns false to signal work is
> not complete. This return value is considered for the NAPI signaling
> by tsnep_poll().

i was only referring to tsnep_xdp_xmit_zc() being void on return. Thing is
that there is currently no way you would tell to napi there is more work
to be done. you should do so if desc_available == batch. That would mean
there might be still descriptors on XSK Tx ring ready to be consumed. NAPI
budget is out of the picture here.