Re: [PATCH v11 4/7] wifi: rtw88: sdio: zero the padding added to a TX transfer

[Luka Gejak]

On Thu Sep 10, 2026 at 4:56 PM CEST, Luka Gejak wrote:
> On Thu Sep 10, 2026 at 4:24 AM CEST, Ping-Ke Shih wrote:
>> luka.gejak@xxxxxxxxx <luka.gejak@xxxxxxxxx> wrote:
>>> From: Luka Gejak <luka.gejak@xxxxxxxxx>
>>> 
>>> rtw_sdio_write_port() rounds the transfer up with sdio_align_size() and
>>> then hands that length to sdio_memcpy_toio() while the skb still only
>>> holds skb->len bytes. The difference, between one and 511 bytes, is read
>>> from beyond the end of the frame and transmitted. Whether it stays
>>> inside the skb's allocation depends on how much tailroom the skb happens
>>> to have, so this is at best sending uninitialised memory over the air.
>>> 
>>> Pad the skb up to the transfer size first. __skb_pad() zeroes the added
>>> bytes, reallocates a cloned skb rather than writing into a buffer a
>>> clone still shares, and leaves skb->len alone, so nothing else in the
>>> transmit path has to change.
>>
>> With __skb_pad(), it might increase CPU usage.
>> Could you roughly measure that?
>>
>
> Measured with the ftrace function profiler over a 30 second saturating
> uplink transfer, 18.6 Mbit/s, four cores:
>
>   function              hits      time(us)   share of one core
>   rtw_sdio_write_port  48358      26839023        89.2%
>   sdio_memcpy_toio     47855      11783994        39.2%
>   __skb_pad            48344        616489         2.1%
>   pskb_expand_head     47869        466036         1.6%
>
> __skb_pad() is about 2% of one core with the link saturated. The whole
> transfer takes 14.8% of the four cores against 0.77% idle, so the
> padding is roughly 3.5% of the CPU the transfer already uses and 2.3%
> of the driver's write path. On throughput it is about 6%, which is the
> A/B in the commit message.
>
> pskb_expand_head() runs on 99% of the calls, so nearly every frame takes
> the reallocating path rather than the memset. That is three quarters of
> the cost; without it the padding would be around 0.5% of one core. I
> have not worked out yet whether the skb is cloned or short of tailroom.
> If it is tailroom it is probably avoidable, and I can chase it as a
> follow up.
>
> I do not think this is a blocker. A few percent seems a fair price for
> not putting uninitialised memory on the air.
>
> One thing to decide, though. The commit message says the padding "costs
> nothing observable", which is too strong given the numbers in that same
> paragraph. Do you think it needs rewording? If so, would you be willing
> to amend it yourself when applying? That seems better than a resend of
> seven patches for one line.
>

Following up on the padding cost, since I said I would find out where it
goes.

It is tailroom, not cloning. Over 70000 padded frames, none were cloned:
frames average 1575 bytes and need about 471 bytes of padding, but arrive
with about 106 bytes of tailroom, so __skb_pad() reallocates on 98% of
them. mac80211 reserves IEEE80211_ENCRYPT_TAILROOM, 18 bytes, and offers
no way for a driver to ask for more on TX; extra_tx_headroom is headroom
and extra_beacon_tailroom is beacons only.

So I tried the other way round, a preallocated per-device buffer: copy
the frame plus the zeros into it and transfer from there, which avoids
the allocation. Interleaved A/B, 20 second uplink runs:

  mode         throughput   CPU (4 cores)   __skb_pad
  __skb_pad    18.6, 18.3      14.3, 15.0    444, 449 ms
  bounce       19.0, 19.8      13.8, 14.3        0 ms

That recovers most of it, roughly 2% of one core and 3 to 5% of
throughput.

I am not proposing it for this series. It is a workaround for the
missing tailroom knob, it costs a copy, and it is only clean where a
lock already serialises the transfer, which is true for this chip but
not for the generic path. If you would rather have it as a separate
patch later, or think the tailroom knob is worth raising with Johannes,
I am happy either way.

Best regards,
Luka Gejak

[...]