| Message ID | 20241122180435.1637479-1-alexthreed@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: brcmfmac: remove misleading log messages | expand |
On 11/22/2024 7:04 PM, Alex Shumsky wrote: > Currently when debug info enabled, dmesg spammed every few minutes with > misleading messages like: > brcmf_netdev_start_xmit phy0-sta0: insufficient headroom (0) > > Do not log this when headroom is actually sufficient. Thanks for your patch. The message may be misleading, but it is actually information that we need to cow the packet. The zero value indicates that this is needed because skb_header_cloned(skb) is true. So it is still useful in my opinion. If you want to make the message less misleading for that case I would be happy to ack the patch. Regards, Arend
On Tue, Nov 26, 2024 at 2:02 PM Arend van Spriel <arend.vanspriel@broadcom.com> wrote: > > On 11/22/2024 7:04 PM, Alex Shumsky wrote: > > Currently when debug info enabled, dmesg spammed every few minutes with > > misleading messages like: > > brcmf_netdev_start_xmit phy0-sta0: insufficient headroom (0) > > > > Do not log this when headroom is actually sufficient. > > Thanks for your patch. The message may be misleading, but it is actually > information that we need to cow the packet. The zero value indicates > that this is needed because skb_header_cloned(skb) is true. So it is > still useful in my opinion. If you want to make the message less > misleading for that case I would be happy to ack the patch. > > Regards, > Arend Thanks for the review and sorry for the delayed response. Do "%s: clone skb header\n" rephrase make sense to you? I have no deep knowledge of this code, and if you think the original message is actually useful, I'm ok to leave a log message as it is. Initially I had guessed it was an unintentional log message because it has misleading text and logs spammed every few minutes - too rarely to consider It as a real performance issue.
On 12/9/2024 9:08 PM, Alex Shumsky wrote: > On Tue, Nov 26, 2024 at 2:02 PM Arend van Spriel > <arend.vanspriel@broadcom.com> wrote: >> >> On 11/22/2024 7:04 PM, Alex Shumsky wrote: >>> Currently when debug info enabled, dmesg spammed every few minutes with >>> misleading messages like: >>> brcmf_netdev_start_xmit phy0-sta0: insufficient headroom (0) >>> >>> Do not log this when headroom is actually sufficient. >> >> Thanks for your patch. The message may be misleading, but it is actually >> information that we need to cow the packet. The zero value indicates >> that this is needed because skb_header_cloned(skb) is true. So it is >> still useful in my opinion. If you want to make the message less >> misleading for that case I would be happy to ack the patch. >> >> Regards, >> Arend > > Thanks for the review and sorry for the delayed response. > Do "%s: clone skb header\n" rephrase make sense to you? I would say: brcmf_dbg(INFO, "%s: %s headroom\n", brcmf_ifname(ifp), head_delta ? "insufficient" : "unmodifiable"); > I have no deep knowledge of this code, and if you think the original message > is actually useful, I'm ok to leave a log message as it is. > Initially I had guessed it was an unintentional log message because it has > misleading text and logs spammed every few minutes - too rarely to consider > It as a real performance issue. If you enable debug prints you should expect performance impact. If you want to capture debug message with negligible performance loss you should use ftrace. Debug prints in brcmfmac are setup as trace events. Regards, Arend
On Thu, Dec 12, 2024 at 12:30 PM Arend van Spriel <arend.vanspriel@broadcom.com> wrote: > > On 12/9/2024 9:08 PM, Alex Shumsky wrote: > > On Tue, Nov 26, 2024 at 2:02 PM Arend van Spriel > > <arend.vanspriel@broadcom.com> wrote: > >> > >> On 11/22/2024 7:04 PM, Alex Shumsky wrote: > >>> Currently when debug info enabled, dmesg spammed every few minutes with > >>> misleading messages like: > >>> brcmf_netdev_start_xmit phy0-sta0: insufficient headroom (0) > >>> > >>> Do not log this when headroom is actually sufficient. > >> > >> Thanks for your patch. The message may be misleading, but it is actually > >> information that we need to cow the packet. The zero value indicates > >> that this is needed because skb_header_cloned(skb) is true. So it is > >> still useful in my opinion. If you want to make the message less > >> misleading for that case I would be happy to ack the patch. > >> > >> Regards, > >> Arend > > > > Thanks for the review and sorry for the delayed response. > > Do "%s: clone skb header\n" rephrase make sense to you? > > I would say: > > brcmf_dbg(INFO, "%s: %s headroom\n", brcmf_ifname(ifp), > head_delta ? "insufficient" : "unmodifiable"); Thanks. Sent [PATCH v2] wifi: brcmfmac: clarify unmodifiable headroom log message. I'm not sure whether I have to link new version with changed subject, I haven't found a way > > I have no deep knowledge of this code, and if you think the original message > > is actually useful, I'm ok to leave a log message as it is. > > Initially I had guessed it was an unintentional log message because it has > > misleading text and logs spammed every few minutes - too rarely to consider > > It as a real performance issue. > > If you enable debug prints you should expect performance impact. If you > want to capture debug message with negligible performance loss you > should use ftrace. Debug prints in brcmfmac are setup as trace events. I expressed my thoughts poorly here. By "performance issue" I meant the clone process itself, not the log messages. I have read "insufficient headroom" message as "look, we need to copy headers, if this happens too often, look into this issue and make it zero-copy" > Regards, > Arend
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c index da72fd2d541f..25c77014af9c 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c @@ -327,8 +327,9 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb, if (skb_headroom(skb) < drvr->hdrlen || skb_header_cloned(skb)) { head_delta = max_t(int, drvr->hdrlen - skb_headroom(skb), 0); - brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n", - brcmf_ifname(ifp), head_delta); + if (head_delta > 0) + brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n", + brcmf_ifname(ifp), head_delta); atomic_inc(&drvr->bus_if->stats.pktcowed); ret = pskb_expand_head(skb, ALIGN(head_delta, NET_SKB_PAD), 0, GFP_ATOMIC);
Currently when debug info enabled, dmesg spammed every few minutes with misleading messages like: brcmf_netdev_start_xmit phy0-sta0: insufficient headroom (0) Do not log this when headroom is actually sufficient. Signed-off-by: Alex Shumsky <alexthreed@gmail.com> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)