Re: [PATCH V4 net-next 6/9] net: ena: use xdp_frame in XDP TX flow

[Shay Agroskin <shayagr@amazon.com>]

Maciej Fijalkowski <maciej.fijalkowski@intel.com> writes:

> On Fri, Dec 04, 2020 at 02:11:12PM +0200, akiyano@amazon.com 
> wrote:
>> From: Arthur Kiyanovski <akiyano@amazon.com>
>> 
>> Rename the ena_xdp_xmit_buff() function to ena_xdp_xmit_frame() 
>> and pass
>> it an xdp_frame struct instead of xdp_buff.
>> This change lays the ground for XDP redirect implementation 
>> which uses
>> xdp_frames when 'xmit'ing packets.
>> 
>> Signed-off-by: Shay Agroskin <shayagr@amazon.com>
>> Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
>> ---
>>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 46 
>>  ++++++++++----------
>>  1 file changed, 23 insertions(+), 23 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c 
>> b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> index 222bb576e30e..cbb07548409a 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> @@ -233,18 +233,18 @@ static int ena_xdp_io_poll(struct 
>> napi_struct *napi, int budget)
>>  	return ret;
>>  }
>>  
>>  ...
>>  	if (verdict == XDP_TX) {
>> -		ena_xdp_xmit_buff(rx_ring->netdev,
>> -				  xdp,
>> -				  rx_ring->qid + 
>> rx_ring->adapter->num_io_queues,
>> -				  rx_info);
>> +		xdpf = xdp_convert_buff_to_frame(xdp);
>
> Similar to Jakub's comment on another patch, 
> xdp_convert_buff_to_frame can
> return NULL and from what I can tell you never check that in
> ena_xdp_xmit_frame.
>

Hi, thanks for reviewing the code (:

Going over xdp_convert_buff_to_frame() it seems (to me) that the 
function fails either
- we're using an AF XDP socket
- the driver failed to leave enough room for xdp_frame and 
  skb_shared_info structs

the first isn't supported by ENA, and the second doesn't seem to 
be possible since the driver leaves enough space on the RX page 
and bpf_xdp_adjust_head()/bpf_xdp_adjust_tail() seem
to make sure enough space is left on the page for the structs.

Nevertheless, the correct approach is to check the return value of 
the function. I'll add it in the next patchset. Thanks

>> +		ena_xdp_xmit_frame(rx_ring->netdev, xdpf,
>> +				   rx_ring->qid + 
>> rx_ring->adapter->num_io_queues);
>>  
>>  		xdp_stat = &rx_ring->rx_stats.xdp_tx;
>>  	} else if (unlikely(verdict == XDP_ABORTED)) {
>> @@ -1521,7 +1521,7 @@ static int ena_xdp_handle_buff(struct 
>> ena_ring *rx_ring, struct xdp_buff *xdp)
>>  	if (unlikely(rx_ring->ena_bufs[0].len > ENA_XDP_MAX_MTU))
>>  		return XDP_DROP;
>>  
>> -	ret = ena_xdp_execute(rx_ring, xdp, rx_info);
>> +	ret = ena_xdp_execute(rx_ring, xdp);
>>  
>>  	/* The xdp program might expand the headers */
>> ...
>>  			 */
>>  			if (xdp_verdict == XDP_TX)
>> -- 
>> 2.23.3
>>