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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Apr 2023 14:03:07 +0200
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     Gerhard Engleder <gerhard@...leder-embedded.com>
CC:     <netdev@...r.kernel.org>, <bpf@...r.kernel.org>,
        <davem@...emloft.net>, <kuba@...nel.org>, <edumazet@...gle.com>,
        <pabeni@...hat.com>, <bjorn@...nel.org>,
        <magnus.karlsson@...el.com>, <jonathan.lemon@...il.com>
Subject: Re: [PATCH net-next v3 6/6] tsnep: Add XDP socket zero-copy TX
 support

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@...leder-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.