netdev - Re: [PATCH net-next 2/2] igc: enable XDP metadata in driver

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, 29 Nov 2021 15:39:04 +0100
From:   Jesper Dangaard Brouer <jbrouer@...hat.com>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
Cc:     brouer@...hat.com, bpf@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <borkmann@...earbox.net>,
        anthony.l.nguyen@...el.com, jesse.brandeburg@...el.com,
        intel-wired-lan@...ts.osuosl.org, magnus.karlsson@...el.com,
        bjorn@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] igc: enable XDP metadata in driver



On 26/11/2021 17.16, Alexander Lobakin wrote:
> From: Jesper Dangaard Brouer <brouer@...hat.com>
> Date: Mon, 15 Nov 2021 21:36:30 +0100
> 
>> Enabling the XDP bpf_prog access to data_meta area is a very small
>> change. Hint passing 'true' to xdp_prepare_buff().
>>
>> The SKB layers can also access data_meta area, which required more
>> driver changes to support. Reviewers, notice the igc driver have two
>> different functions that can create SKBs, depending on driver config.
>>
>> Hint for testers, ethtool priv-flags legacy-rx enables
>> the function igc_construct_skb()
>>
>>   ethtool --set-priv-flags DEV legacy-rx on
>>
>> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
>> ---
>>   drivers/net/ethernet/intel/igc/igc_main.c |   29 +++++++++++++++++++----------
>>   1 file changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index 76b0a7311369..b516f1b301b4 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -1718,24 +1718,26 @@ static void igc_add_rx_frag(struct igc_ring *rx_ring,
>>   
>>   static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring,
>>   				     struct igc_rx_buffer *rx_buffer,
>> -				     union igc_adv_rx_desc *rx_desc,
>> -				     unsigned int size)
>> +				     struct xdp_buff *xdp)
>>   {
>> -	void *va = page_address(rx_buffer->page) + rx_buffer->page_offset;
>> +	unsigned int size = xdp->data_end - xdp->data;
>>   	unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size);
>> +	unsigned int metasize = xdp->data - xdp->data_meta;
>>   	struct sk_buff *skb;
>>   
>>   	/* prefetch first cache line of first page */
>> -	net_prefetch(va);
>> +	net_prefetch(xdp->data);
> 
> I'd prefer prefetching xdp->data_meta here. GRO layer accesses it.
> Maximum meta size for now is 32, so at least 96 bytes of the frame
> will stil be prefetched.

Prefetch works for "full" cachelines. Intel CPUs often prefect two 
cache-lines, when doing this, thus I guess we still get xdp->data.

I don't mind prefetching xdp->data_meta, but (1) I tried to keep the 
change minimal as current behavior was data area I kept that. (2) 
xdp->data starts on a cacheline and we know NIC hardware have touched 
that, it is not a full-cache-miss due to DDIO/DCA it is known to be in 
L3 cache (gain is around 2-3 ns in my machine for data prefetch).
Given this is only a 2.5 Gbit/s driver/HW I doubt this make any difference.

Tony is it worth resending a V2 of this patch?

>>   
>>   	/* build an skb around the page buffer */
>> -	skb = build_skb(va - IGC_SKB_PAD, truesize);
>> +	skb = build_skb(xdp->data_hard_start, truesize);
>>   	if (unlikely(!skb))
>>   		return NULL;
>>   
>>   	/* update pointers within the skb to store the data */
>> -	skb_reserve(skb, IGC_SKB_PAD);
>> +	skb_reserve(skb, xdp->data - xdp->data_hard_start);
>>   	__skb_put(skb, size);
>> +	if (metasize)
>> +		skb_metadata_set(skb, metasize);
>>   
>>   	igc_rx_buffer_flip(rx_buffer, truesize);
>>   	return skb;
>> @@ -1746,6 +1748,7 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
>>   					 struct xdp_buff *xdp,
>>   					 ktime_t timestamp)
>>   {
>> +	unsigned int metasize = xdp->data - xdp->data_meta;
>>   	unsigned int size = xdp->data_end - xdp->data;
>>   	unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size);
>>   	void *va = xdp->data;
>> @@ -1756,7 +1759,7 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
>>   	net_prefetch(va);
> 
> ...here as well.
>