| Message ID | 20250313134942.52ff54a140ec.If390bbdc46904cf451256ba989d7a056c457af6e@changeid (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [RFC] wifi: free SKBTX_WIFI_STATUS skb tx_flags flag | expand |
On Thu, 13 Mar 2025 13:49:39 +0100 Johannes Berg wrote: > Someone mentioned today at netdevconf that we've run out of Don't recall the mention, but I'd guess maybe Jason Xing during his timestamping talk? Change itself LGTM! > tx_flags in the skb_shinfo(). Gain one bit back by removing > the wifi bit. We should be able to do that because the only > userspace application for it (hostapd) doesn't change the > setting on the socket, it just uses different sockets, and > normally doesn't even use this any more, sending the frames > over nl80211 instead.
On Mon, 2025-03-24 at 08:53 -0700, Jakub Kicinski wrote: > On Thu, 13 Mar 2025 13:49:39 +0100 Johannes Berg wrote: > > Someone mentioned today at netdevconf that we've run out of > > Don't recall the mention, but I'd guess maybe Jason Xing during > his timestamping talk? Change itself LGTM! Yeah, I think so. Timing would be right too, his talk was around noon that day :) I guess I can merge it through wireless for 6.16, unless you have any concerns? I can also resend it for net-next after the merge window closes, the wireless drivers/stack changes are almost certainly not going to have any conflicts, but overall it's trivial anyway. johannes
On Mon, 24 Mar 2025 16:57:52 +0100 Johannes Berg wrote: > On Mon, 2025-03-24 at 08:53 -0700, Jakub Kicinski wrote: > > On Thu, 13 Mar 2025 13:49:39 +0100 Johannes Berg wrote: > > > Someone mentioned today at netdevconf that we've run out of > > > > Don't recall the mention, but I'd guess maybe Jason Xing during > > his timestamping talk? Change itself LGTM! > > Yeah, I think so. Timing would be right too, his talk was around noon > that day :) > > I guess I can merge it through wireless for 6.16, unless you have any > concerns? I can also resend it for net-next after the merge window > closes, the wireless drivers/stack changes are almost certainly not > going to have any conflicts, but overall it's trivial anyway. 6.16 makes sense; no preference on the tree, whichever's easier.
On Mon, Mar 24, 2025 at 11:55 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 13 Mar 2025 13:49:39 +0100 Johannes Berg wrote: > > Someone mentioned today at netdevconf that we've run out of > > Don't recall the mention, but I'd guess maybe Jason Xing during > his timestamping talk? Change itself LGTM! Yes, I did mention it. Sorry for missing this commit. Considering it's too late for me, I promise I will review tomorrow morning. Thanks, Jason > > > tx_flags in the skb_shinfo(). Gain one bit back by removing > > the wifi bit. We should be able to do that because the only > > userspace application for it (hostapd) doesn't change the > > setting on the socket, it just uses different sockets, and > > normally doesn't even use this any more, sending the frames > > over nl80211 instead. >
On Thu, Mar 13, 2025 at 9:01 PM Johannes Berg <johannes@sipsolutions.net> wrote: > > From: Johannes Berg <johannes.berg@intel.com> > > Someone mentioned today at netdevconf that we've run out of > tx_flags in the skb_shinfo(). Gain one bit back by removing > the wifi bit. We should be able to do that because the only > userspace application for it (hostapd) doesn't change the > setting on the socket, it just uses different sockets, and > normally doesn't even use this any more, sending the frames > over nl80211 instead. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> Thanks for working on this. After net-next is open, I will use this bit to finish the bpf timestamping in the rx path :) Reviewed-by: Jason Xing <kerneljasonxing@gmail.com> > --- > drivers/net/wireless/ath/wil6210/txrx.h | 3 ++- > drivers/net/wireless/marvell/mwifiex/main.c | 3 ++- > include/linux/skbuff.h | 3 --- > include/net/sock.h | 2 -- > net/mac80211/mesh.c | 3 ++- > net/mac80211/tx.c | 9 ++++----- > 6 files changed, 10 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/wireless/ath/wil6210/txrx.h b/drivers/net/wireless/ath/wil6210/txrx.h > index 689f68d89a44..33ccd0b248d4 100644 > --- a/drivers/net/wireless/ath/wil6210/txrx.h > +++ b/drivers/net/wireless/ath/wil6210/txrx.h > @@ -7,6 +7,7 @@ > #ifndef WIL6210_TXRX_H > #define WIL6210_TXRX_H > > +#include <net/sock.h> > #include "wil6210.h" > #include "txrx_edma.h" > > @@ -617,7 +618,7 @@ static inline bool wil_need_txstat(struct sk_buff *skb) > const u8 *da = wil_skb_get_da(skb); > > return is_unicast_ether_addr(da) && skb->sk && > - (skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS); > + sock_flag(skb->sk, SOCK_WIFI_STATUS); > } > > static inline void wil_consume_skb(struct sk_buff *skb, bool acked) > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c > index 45eecb5f643b..058687793a10 100644 > --- a/drivers/net/wireless/marvell/mwifiex/main.c > +++ b/drivers/net/wireless/marvell/mwifiex/main.c > @@ -6,6 +6,7 @@ > */ > > #include <linux/suspend.h> > +#include <net/sock.h> > > #include "main.h" > #include "wmm.h" > @@ -943,7 +944,7 @@ mwifiex_hard_start_xmit(struct sk_buff *skb, struct net_device *dev) > multicast = is_multicast_ether_addr(skb->data); > > if (unlikely(!multicast && skb->sk && > - skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS && > + sock_flag(skb->sk, SOCK_WIFI_STATUS) && > priv->adapter->fw_api_ver == MWIFIEX_FW_V15)) > skb = mwifiex_clone_skb_for_tx_status(priv, > skb, > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index 14517e95a46c..a8638c8a53b4 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -481,9 +481,6 @@ enum { > /* reserved */ > SKBTX_RESERVED = 1 << 3, It might conflict with the bluetooth commit [1], I presume. [1]: https://web.git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=c6c3dc495a6ce5b9dfed4df08c3220207e7103bd > > - /* generate wifi status information (where possible) */ > - SKBTX_WIFI_STATUS = 1 << 4, > - Better use SKBTX_RESERVED. No strong preference here since I'm going to use it. > /* determine hardware time stamp based on time or cycles */ > SKBTX_HW_TSTAMP_NETDEV = 1 << 5, > > diff --git a/include/net/sock.h b/include/net/sock.h > index 8daf1b3b12c6..2668c3ed45ef 100644 > --- a/include/net/sock.h > +++ b/include/net/sock.h > @@ -2700,8 +2700,6 @@ static inline void _sock_tx_timestamp(struct sock *sk, > *tskey = atomic_inc_return(&sk->sk_tskey) - 1; > } > } > - if (unlikely(sock_flag(sk, SOCK_WIFI_STATUS))) > - *tx_flags |= SKBTX_WIFI_STATUS; > } > > static inline void sock_tx_timestamp(struct sock *sk, > diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c > index 974081324aa4..e77e623c8b77 100644 > --- a/net/mac80211/mesh.c > +++ b/net/mac80211/mesh.c > @@ -8,6 +8,7 @@ > > #include <linux/slab.h> > #include <linux/unaligned.h> > +#include <net/sock.h> > #include "ieee80211_i.h" > #include "mesh.h" > #include "wme.h" > @@ -776,7 +777,7 @@ bool ieee80211_mesh_xmit_fast(struct ieee80211_sub_if_data *sdata, > if (ethertype < ETH_P_802_3_MIN) > return false; > > - if (skb->sk && skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS) > + if (skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS)) > return false; > > if (skb->ip_summed == CHECKSUM_PARTIAL) { > diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c > index 20179db88c4a..b75f72fbefd9 100644 > --- a/net/mac80211/tx.c > +++ b/net/mac80211/tx.c > @@ -26,6 +26,7 @@ > #include <net/codel_impl.h> > #include <linux/unaligned.h> > #include <net/fq_impl.h> > +#include <net/sock.h> > #include <net/gso.h> > > #include "ieee80211_i.h" > @@ -2876,8 +2877,7 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata, > } > > if (unlikely(!multicast && > - ((skb->sk && > - skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS) || > + ((skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS)) || > ctrl_flags & IEEE80211_TX_CTL_REQ_TX_STATUS))) > info_id = ieee80211_store_ack_skb(local, skb, &info_flags, > cookie); > @@ -3774,7 +3774,7 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata, > return false; > > /* don't handle TX status request here either */ > - if (skb->sk && skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS) > + if (skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS)) > return false; > > if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) { > @@ -4664,8 +4664,7 @@ static void ieee80211_8023_xmit(struct ieee80211_sub_if_data *sdata, > memcpy(IEEE80211_SKB_CB(seg), info, sizeof(*info)); > } > > - if (unlikely(skb->sk && > - skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS)) { > + if (unlikely(skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS))) { > info->status_data = ieee80211_store_ack_skb(local, skb, > &info->flags, NULL); > if (info->status_data) > -- > 2.48.1 > >
On Tue, 2025-03-25 at 00:53 +0800, Jason Xing wrote: > > Thanks for working on this. After net-next is open, I will use this > bit to finish the bpf timestamping in the rx path :) :) > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -481,9 +481,6 @@ enum { > > /* reserved */ > > SKBTX_RESERVED = 1 << 3, > > It might conflict with the bluetooth commit [1], I presume. > > [1]: https://web.git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=c6c3dc495a6ce5b9dfed4df08c3220207e7103bd True, just context though, we can deal with that. > > > > - /* generate wifi status information (where possible) */ > > - SKBTX_WIFI_STATUS = 1 << 4, > > - > > Better use SKBTX_RESERVED. No strong preference here since I'm going to use it. I can't have two called SKBTX_RESERVED, but I'm not sure it's worth renaming it rather than removing? No strong opinion though. The context conflict will happen either way ;) johannes
On Tue, Mar 25, 2025 at 12:56 AM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Tue, 2025-03-25 at 00:53 +0800, Jason Xing wrote: > > > > Thanks for working on this. After net-next is open, I will use this > > bit to finish the bpf timestamping in the rx path :) > > :) > > > > --- a/include/linux/skbuff.h > > > +++ b/include/linux/skbuff.h > > > @@ -481,9 +481,6 @@ enum { > > > /* reserved */ > > > SKBTX_RESERVED = 1 << 3, > > > > It might conflict with the bluetooth commit [1], I presume. > > > > [1]: https://web.git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=c6c3dc495a6ce5b9dfed4df08c3220207e7103bd > > True, just context though, we can deal with that. > > > > > > > - /* generate wifi status information (where possible) */ > > > - SKBTX_WIFI_STATUS = 1 << 4, > > > - > > > > Better use SKBTX_RESERVED. No strong preference here since I'm going to use it. > > I can't have two called SKBTX_RESERVED, but I'm not sure it's worth > renaming it rather than removing? No strong opinion though. The context > conflict will happen either way ;) Sure, we can ignore it. As I mentioned, this released bit will be used soon :) > > johannes >
diff --git a/drivers/net/wireless/ath/wil6210/txrx.h b/drivers/net/wireless/ath/wil6210/txrx.h index 689f68d89a44..33ccd0b248d4 100644 --- a/drivers/net/wireless/ath/wil6210/txrx.h +++ b/drivers/net/wireless/ath/wil6210/txrx.h @@ -7,6 +7,7 @@ #ifndef WIL6210_TXRX_H #define WIL6210_TXRX_H +#include <net/sock.h> #include "wil6210.h" #include "txrx_edma.h" @@ -617,7 +618,7 @@ static inline bool wil_need_txstat(struct sk_buff *skb) const u8 *da = wil_skb_get_da(skb); return is_unicast_ether_addr(da) && skb->sk && - (skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS); + sock_flag(skb->sk, SOCK_WIFI_STATUS); } static inline void wil_consume_skb(struct sk_buff *skb, bool acked) diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c index 45eecb5f643b..058687793a10 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.c +++ b/drivers/net/wireless/marvell/mwifiex/main.c @@ -6,6 +6,7 @@ */ #include <linux/suspend.h> +#include <net/sock.h> #include "main.h" #include "wmm.h" @@ -943,7 +944,7 @@ mwifiex_hard_start_xmit(struct sk_buff *skb, struct net_device *dev) multicast = is_multicast_ether_addr(skb->data); if (unlikely(!multicast && skb->sk && - skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS && + sock_flag(skb->sk, SOCK_WIFI_STATUS) && priv->adapter->fw_api_ver == MWIFIEX_FW_V15)) skb = mwifiex_clone_skb_for_tx_status(priv, skb, diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 14517e95a46c..a8638c8a53b4 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -481,9 +481,6 @@ enum { /* reserved */ SKBTX_RESERVED = 1 << 3, - /* generate wifi status information (where possible) */ - SKBTX_WIFI_STATUS = 1 << 4, - /* determine hardware time stamp based on time or cycles */ SKBTX_HW_TSTAMP_NETDEV = 1 << 5, diff --git a/include/net/sock.h b/include/net/sock.h index 8daf1b3b12c6..2668c3ed45ef 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -2700,8 +2700,6 @@ static inline void _sock_tx_timestamp(struct sock *sk, *tskey = atomic_inc_return(&sk->sk_tskey) - 1; } } - if (unlikely(sock_flag(sk, SOCK_WIFI_STATUS))) - *tx_flags |= SKBTX_WIFI_STATUS; } static inline void sock_tx_timestamp(struct sock *sk, diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c index 974081324aa4..e77e623c8b77 100644 --- a/net/mac80211/mesh.c +++ b/net/mac80211/mesh.c @@ -8,6 +8,7 @@ #include <linux/slab.h> #include <linux/unaligned.h> +#include <net/sock.h> #include "ieee80211_i.h" #include "mesh.h" #include "wme.h" @@ -776,7 +777,7 @@ bool ieee80211_mesh_xmit_fast(struct ieee80211_sub_if_data *sdata, if (ethertype < ETH_P_802_3_MIN) return false; - if (skb->sk && skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS) + if (skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS)) return false; if (skb->ip_summed == CHECKSUM_PARTIAL) { diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 20179db88c4a..b75f72fbefd9 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -26,6 +26,7 @@ #include <net/codel_impl.h> #include <linux/unaligned.h> #include <net/fq_impl.h> +#include <net/sock.h> #include <net/gso.h> #include "ieee80211_i.h" @@ -2876,8 +2877,7 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata, } if (unlikely(!multicast && - ((skb->sk && - skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS) || + ((skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS)) || ctrl_flags & IEEE80211_TX_CTL_REQ_TX_STATUS))) info_id = ieee80211_store_ack_skb(local, skb, &info_flags, cookie); @@ -3774,7 +3774,7 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata, return false; /* don't handle TX status request here either */ - if (skb->sk && skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS) + if (skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS)) return false; if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) { @@ -4664,8 +4664,7 @@ static void ieee80211_8023_xmit(struct ieee80211_sub_if_data *sdata, memcpy(IEEE80211_SKB_CB(seg), info, sizeof(*info)); } - if (unlikely(skb->sk && - skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS)) { + if (unlikely(skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS))) { info->status_data = ieee80211_store_ack_skb(local, skb, &info->flags, NULL); if (info->status_data)