Message-ID: <9f9331ac-2ae0-4a92-b57b-d63bac858379@intel.com>
Date: Tue, 19 Aug 2025 12:53:42 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Jesper Dangaard Brouer <hawk@...nel.org>, Tony Nguyen
	<anthony.l.nguyen@...el.com>, <ast@...nel.org>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <netdev@...r.kernel.org>
CC: <maciej.fijalkowski@...el.com>, <magnus.karlsson@...el.com>,
	<andrew+netdev@...n.ch>, <daniel@...earbox.net>, <john.fastabend@...il.com>,
	<sdf@...ichev.me>, <bpf@...r.kernel.org>, <horms@...nel.org>,
	<przemyslaw.kitszel@...el.com>, <aleksander.lobakin@...el.com>,
	<jaroslav.pulchart@...ddata.com>, <jdamato@...tly.com>,
	<christoph.petrausch@...pl.com>, Rinitha S <sx.rinitha@...el.com>, "Priya
 Singh" <priyax.singh@...el.com>, Eelco Chaudron <echaudro@...hat.com>,
	<edumazet@...gle.com>
Subject: Re: [PATCH net 3/6] ice: fix Rx page leak on multi-buffer frames



On 8/19/2025 9:44 AM, Jesper Dangaard Brouer wrote:
> 
> 
> On 19/08/2025 02.38, Jacob Keller wrote:
>>
>>
>> On 8/18/2025 4:05 AM, Jesper Dangaard Brouer wrote:
>>> On 15/08/2025 22.41, Tony Nguyen wrote:
>>>> This has the advantage that we also no longer need to track or cache the
>>>> number of fragments in the rx_ring, which saves a few bytes in the ring.
>>>>
>>>
>>> Have anyone tested the performance impact for XDP_DROP ?
>>> (with standard non-multi-buffer frames)
>>>
>>> Below code change will always touch cache-lines in shared_info area.
>>> Before it was guarded with a xdp_buff_has_frags() check.
>>>
>>
>> I did some basic testing with XDP_DROP previously using the xdp-bench
>> tool, and do not recall notice an issue. I don't recall the actual
>> numbers now though, so I did some quick tests again.
>>
>> without patch...
>>
>> Client:
>> $ iperf3 -u -c 192.168.93.1 -t86400 -l1200 -P20 -b5G
>> ...
>> [SUM]  10.00-10.33  sec   626 MBytes  16.0 Gbits/sec  546909
>>
>> $ iperf3 -s -B 192.168.93.1%ens260f0
>> [SUM]   0.00-10.00  sec  17.7 GBytes  15.2 Gbits/sec  0.011 ms
>> 9712/15888183 (0.061%)  receiver
>>
>> $ xdp-bench drop ens260f0
>> Summary                 1,778,935 rx/s                  0 err/s
>> Summary                 2,041,087 rx/s                  0 err/s
>> Summary                 2,005,052 rx/s                  0 err/s
>> Summary                 1,918,967 rx/s                  0 err/s
>>
>> with patch...
>>
>> Client:
>> $ iperf3 -u -c 192.168.93.1 -t86400 -l1200 -P20 -b5G
>> ...
>> [SUM]  78.00-78.90  sec  2.01 GBytes  19.1 Gbits/sec  1801284
>>
>> Server:
>> $ iperf3 -s -B 192.168.93.1%ens260f0
>> [SUM]  77.00-78.00  sec  2.14 GBytes  18.4 Gbits/sec  0.012 ms
>> 9373/1921186 (0.49%)
>>
>> xdp-bench:
>> $ xdp-bench drop ens260f0
>> Dropping packets on ens260f0 (ifindex 8; driver ice)
>> Summary                 1,910,918 rx/s                  0 err/s
>> Summary                 1,866,562 rx/s                  0 err/s
>> Summary                 1,901,233 rx/s                  0 err/s
>> Summary                 1,859,854 rx/s                  0 err/s
>> Summary                 1,593,493 rx/s                  0 err/s
>> Summary                 1,891,426 rx/s                  0 err/s
>> Summary                 1,880,673 rx/s                  0 err/s
>> Summary                 1,866,043 rx/s                  0 err/s
>> Summary                 1,872,845 rx/s                  0 err/s
>>
>>
>> I ran a few times and it seemed to waffle a bit around 15Gbit/sec to
>> 20Gbit/sec, with throughput varying regardless of which patch applied. I
>> actually tended to see slightly higher numbers with this fix applied,
>> but it was not consistent and hard to measure.
>>
> 
> Above testing is not a valid XDP_DROP test.
> 

Fair. I'm no XDP expert, so I have a lot to learn here :)

> The packet generator need to be much much faster, as XDP_DROP is for
> DDoS protection use-cases (one of Cloudflare's main products).
> 
> I recommend using the script for pktgen in kernel tree:
>   samples/pktgen/pktgen_sample03_burst_single_flow.sh
> 
> Example:
>   ./pktgen_sample03_burst_single_flow.sh -vi mlx5p2 -d 198.18.100.1 -m 
> b4:96:91:ad:0b:09 -t $(nproc)
> 
> 
>> without the patch:
> 
> On my testlab with CPU: AMD EPYC 9684X (SRSO=IBPB) running:
>   - sudo ./xdp-bench drop ice4  # (defaults to no-touch)
> 
> XDP_DROP (with no-touch)
>   Without patch :  54,052,300 rx/s = 18.50 nanosec/packet
>   With the patch:  33,420,619 rx/s = 29.92 nanosec/packet
>   Diff: 11.42 nanosec
> 

Oof. Yea, thats not good.

> Using perf stat I can see an increase in cache-misses.
> 
> The difference is less, if we read-packet data, running:
>   - sudo ./xdp-bench drop ice4 --packet-operation read-data
> 
> XDP_DROP (with read-data)
>   Without patch :  27,200,683 rx/s = 36.76 nanosec/packet
>   With the patch:  24,348,751 rx/s = 41.07 nanosec/packet
>   Diff: 4.31 nanosec
> 
> On this CPU we don't have DDIO/DCA, so we take a big hit reading the
> packet data in XDP.  This will be needed by our DDoS bpf_prog.
> The nanosec diff isn't the same, so it seem this change can hide a
> little behind the cache-miss in the XDP bpf_prog.
> 
> 
>> Without xdp-bench running the XDP program, top showed a CPU usage of
>> 740% and an ~86 idle score.
>>
> 
> We don't want a scaling test for this. For this XDP_DROP/DDoS test we
> want to target a single CPU. This is easiest done by generating a single
> flow (hint pktgen script is called _single_flow). We want to see a
> single CPU running ksoftirqd 100% of the time.
> 

Ok.

>>
>> I'm not certain, but reading the helpers it might be correct to do
>> something like this:
>>
>> if (unlikely(xdp_buff_has_frags(xdp)))
>>    nr_frags = xdp_get_shared_info_from_buff(xdp)->nr_frags;
>> else
>>    nr_frags = 1
> 
> Yes, that looks like a correct pattern.
> 
>> either in the driver code or by adding a new xdp helper function.
>>
>> I'm not sure its worth it though. We have pending work from our
>> development team to refactor ice to use page pool and switch to libeth
>> XDP helpers which eliminates all of this driver-specific logic.
> 
> Please do proper testing of XDP_DROP case when doing this change.
> 
>> I don't personally think its worth holding up this series and this
>> important memory leak fix for a minor potential code change that I can't
>> measure an obvious improvement on.
> 
> IMHO you included an optimization (that wasn't a gain) in a fix patch.
> I think you can fix the memory leak without the "optimization" part.
> 

It wasn't intended as an optimization in any case, but me trying to make
it easier to keep track of what the driver was doing, but obviously
ended up regressing here.

@Jakub, @Tony, I guess we'll have to drop this patch from the series,
and I'll work on a v2 to avoid this regression.

> pw-bot: cr
> 
> --Jesper
> 



Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)

Powered by blists - more mailing lists