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

[Gerhard Engleder <gerhard@engleder-embedded.com>]

On 24.04.23 14:03, Maciej Fijalkowski wrote:
> 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.

Now I got it. Pending zero-copy work shall be signaled to napi directly.
In current implementation it would be processed if TX interrupt signals
completed descriptors. By signaling napi directly, the TX interrupt
could be prevented. In my opinion both options are possible, but the
performance may differ. I will check if telling napi that there is more
work to be done would improve performance.