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

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:

quoted

luka.gejak@linux.dev [off-list ref] wrote:

quoted

From: Luka Gejak <redacted>

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

[...]