* Re: [PATCH net-next v1 0/2] Reuse threaded NAPI kthread across napi_del()/napi_add().
From: Mina Almasry @ 2026-06-30 17:38 UTC (permalink / raw)
To: Jakub Kicinski, Harshitha Ramamurthy, Jordan Rhee
Cc: Shuhao Tan, David S . Miller, Eric Dumazet, Paolo Abeni,
Simon Horman, Andrew Lunn, Shuah Khan, Samiullah Khawaja,
Kuniyuki Iwashima, netdev, linux-kernel, linux-kselftest
In-Reply-To: <20260629181955.00e63b61@kernel.org>
On Mon, Jun 29, 2026 at 6:19 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 29 Jun 2026 17:47:03 -0700 Shuhao Tan wrote:
> > > Send a netdev Netlink notification when NAPI is re-created and
> > > let the userspace re-apply the settings?
> >
> > It feels surprising that the userspace needs to reconfigure thread
> > properties when changing NIC configurations unrelated to threading.
> > Another downside is that when userspace configures NIC configurations
> > in quick succession, re-application becomes messy because a previous
> > re-application might still be in progress when the thread is gone.
>
> Can you explain more about your deployment and system configuration
> flow? We may be adding micro optimizations when the problem is that
> we recreate the NAPIs in the first place.
We have an AF_XDP application with extremely low latency and jitter
requirements running on our servers. Sami developed busypolling
threaded napi for them. Since it's an AF_XDP application, they attach
their umem to specific RX queues, and then configure threaded NAPI
busypolling to achieve low latency. That involves using the Netlink
API to set the threaded/busypolling property, grabbing the kthread
PID, and setting some properties on the kthread. Concretely, something
like:
```
local napi_id
napi_id=$(call_ynl --output-json --do queue-get \
--json "{\"ifindex\": ${ifindex}, \"id\": ${q_id}, \"type\": \"rx\"}" | \
jq -r '."napi-id"')
echo "Enabling busypolling for queue ${q_id} (NAPI ${napi_id}) on CPU ${cpu}"
call_ynl --do napi-set --json "{\"id\": \"${napi_id}\",
\"threaded\": \"busy-poll\"}" >/dev/null
local napi_kthread_pid
napi_kthread_pid=$(call_ynl --do napi-get --output-json \
--json "{\"id\": \"${napi_id}\"}" | jq -r '."pid" // empty')
taskset -pc "${cpu}" "${napi_kthread_pid}" >/dev/null
```
The bug is that the taskset configurations disappear when the user
runs an unrelated ethtool command. Yes, the root cause is that an
unrelated ethtool driver config on GVE, gve_adjust_config() will
recreate the NAPIs. My understanding is that NAPIs are recreated on
ethtool commands as WAI and standard upstream driver behavior; is that
not correct? If we held onto the same NAPIs during driver reconfigs,
this issue would indeed be fixed. Is holding onto the same NAPI during
driver reconfigurations an appropriate/feasible fix here?
Other ideas, FWIW:
1. We could add an optional netlink argument, "remember_napi_kthread,"
that customizes this behavior for users who want to remember thread
config.
2. We add an optional netlink argument: "bind_napi_kthread_to_cpu"
that not only sets threaded but also binds the created thread to a
specific cpu set, and then include that cpu set as part of the
napi_config so that's it's remembered on reconfigs.
But I think the best options are (a) not recreating napis during
driver configs or (b) this patch to park/unpark the thread, TBH.
--
Thanks,
Mina
^ permalink raw reply
* Re: [PATCH net v3] octeontx2-pf: check DMAC extraction support before filtering
From: Harshitha Ramamurthy @ 2026-06-30 17:38 UTC (permalink / raw)
To: nshettyj
Cc: netdev, linux-kernel, sgoutham, gakula, sbhatta, hkelam,
bbhushan2, andrew+netdev, davem, edumazet, kuba, pabeni, naveenm,
tduszynski, sumang
In-Reply-To: <20260630162622.2667086-1-nshettyj@marvell.com>
On Tue, Jun 30, 2026 at 9:26 AM <nshettyj@marvell.com> wrote:
>
> From: Suman Ghosh <sumang@marvell.com>
>
> Currently, configuring a VF MAC address via the PF (e.g., 'ip link
> set <pf> vf 0 mac <mac>') blindly attempts to install a DMAC-based
> hardware filter. However, the hardware parser profile might not
> support DMAC extraction.
>
> Check if the hardware parsing profile supports DMAC extraction
> before adding the filter. Additionally, emit a warning message
> to inform the operator if the MAC filter installation fails due
> to missing DMAC extraction support.
>
> Fixes: f0c2982aaf98 ("octeontx2-pf: Add support for SR-IOV management functions")
> Signed-off-by: Suman Ghosh <sumang@marvell.com>
> Signed-off-by: Nitin Shetty J <nshettyj@marvell.com>
>
> ---
> v3:
> - Update config->mac only after hardware programming succeeds in
> otx2_set_vf_mac().
> v2:
> - Move the DMAC extraction check from otx2_set_vf_mac() into
> otx2_do_set_vf_mac() which already holds pf->mbox.lock, so all
> mbox operations are under a single lock/unlock pair. All error
> paths now use the existing goto-out pattern, eliminating the
> scattered mutex_unlock() + return calls from v1.
> - Return -EOPNOTSUPP instead of 0 when DMAC extraction is not
> supported, so the caller gets an explicit error rather than a
> silent success.
> ---
> .../ethernet/marvell/octeontx2/nic/otx2_pf.c | 44 ++++++++++++++++---
> 1 file changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> index b63df5737ff2..697570765957 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> @@ -2517,10 +2517,43 @@ EXPORT_SYMBOL(otx2_config_hwtstamp_set);
>
> static int otx2_do_set_vf_mac(struct otx2_nic *pf, int vf, const u8 *mac)
> {
> + struct npc_get_field_status_req *freq;
> + struct npc_get_field_status_rsp *frsp;
> struct npc_install_flow_req *req;
> int err;
>
> mutex_lock(&pf->mbox.lock);
> +
> + /* Skip installing the DMAC filter if the hardware parser profile
> + * does not support DMAC extraction.
> + */
> + freq = otx2_mbox_alloc_msg_npc_get_field_status(&pf->mbox);
> + if (!freq) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + freq->field = NPC_DMAC;
> + if (otx2_sync_mbox_msg(&pf->mbox)) {
> + err = -EINVAL;
> + goto out;
> + }
> +
> + frsp = (struct npc_get_field_status_rsp *)otx2_mbox_get_rsp
> + (&pf->mbox.mbox, 0, &freq->hdr);
> + if (IS_ERR(frsp)) {
> + err = PTR_ERR(frsp);
> + goto out;
> + }
> +
> + if (!frsp->enable) {
> + netdev_warn(pf->netdev,
> + "VF %d MAC filter not installed: DMAC extraction not supported by parser profile\n",
> + vf);
> + err = -EOPNOTSUPP;
> + goto out;
> + }
> +
> req = otx2_mbox_alloc_msg_npc_install_flow(&pf->mbox);
> if (!req) {
> err = -ENOMEM;
> @@ -2559,13 +2592,12 @@ static int otx2_set_vf_mac(struct net_device *netdev, int vf, u8 *mac)
> if (!is_valid_ether_addr(mac))
> return -EINVAL;
>
> - config = &pf->vf_configs[vf];
> - ether_addr_copy(config->mac, mac);
> -
> ret = otx2_do_set_vf_mac(pf, vf, mac);
> - if (ret == 0)
> - dev_info(&pdev->dev,
> - "Load/Reload VF driver\n");
> + if (ret == 0) {
> + config = &pf->vf_configs[vf];
> + ether_addr_copy(config->mac, mac);
> + dev_info(&pdev->dev, "Load/Reload VF driver\n");
> + }
This is a valid change and would have been nice to mention it in the
commit message. Either way:
Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
>
> return ret;
> }
> --
> 2.48.1
>
^ permalink raw reply
* [PATCH net v2] net: qualcomm: rmnet: validate MAP frame length before ingress parsing
From: Xiang Mei @ 2026-06-30 17:41 UTC (permalink / raw)
To: subash.a.kasiviswanathan, sean.tranchetti, netdev
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, linux-kernel,
bestswngs, Xiang Mei
When ingress deaggregation is disabled, rmnet_map_ingress_handler() passes
the skb straight to __rmnet_map_ingress_handler(), skipping the length
validation that rmnet_map_deaggregate() performs on the aggregated path.
The parser then dereferences the MAP header and csum header/trailer based on
the on-wire pkt_len without checking skb->len, so a short frame is read out
of bounds:
BUG: KASAN: slab-out-of-bounds in rmnet_map_checksum_downlink_packet
Read of size 1 at addr ffff88801118ed00 by task exploit/147
Call Trace:
...
rmnet_map_checksum_downlink_packet (drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:413)
__rmnet_map_ingress_handler (drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c:96)
rmnet_rx_handler (drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c:129)
__netif_receive_skb_core.constprop.0 (net/core/dev.c:6089)
netif_receive_skb (net/core/dev.c:6460)
tun_get_user (drivers/net/tun.c:1955)
tun_chr_write_iter (drivers/net/tun.c:2001)
vfs_write (fs/read_write.c:688)
ksys_write (fs/read_write.c:740)
do_syscall_64 (arch/x86/entry/syscall_64.c:94)
...
Factor that validation out of rmnet_map_deaggregate() into
rmnet_map_validate_packet_len() and run it on the no-aggregation path too.
The MAP header is bounds-checked first, since this path can receive a frame
shorter than the header.
Fixes: ceed73a2cf4a ("drivers: net: ethernet: qualcomm: rmnet: Initial implementation")
Reported-by: Weiming Shi <bestswngs@gmail.com>
Suggested-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
Signed-off-by: Xiang Mei <xmei5@asu.edu>
---
v2: Validate on the no-aggregation path by reusing the deaggregation
length checks (factored into rmnet_map_validate_packet_len()) instead
of adding separate pskb_may_pull() guards in __rmnet_map_ingress_handler().
.../ethernet/qualcomm/rmnet/rmnet_handlers.c | 5 +-
.../net/ethernet/qualcomm/rmnet/rmnet_map.h | 1 +
.../ethernet/qualcomm/rmnet/rmnet_map_data.c | 72 ++++++++++---------
3 files changed, 45 insertions(+), 33 deletions(-)
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index 9f3479500f85..d055a2628d8c 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -126,7 +126,10 @@ rmnet_map_ingress_handler(struct sk_buff *skb,
consume_skb(skb);
} else {
- __rmnet_map_ingress_handler(skb, port);
+ if (rmnet_map_validate_packet_len(skb, port))
+ __rmnet_map_ingress_handler(skb, port);
+ else
+ kfree_skb(skb);
}
}
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index b70284095568..60ca8b780c88 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -59,5 +59,6 @@ void rmnet_map_tx_aggregate_init(struct rmnet_port *port);
void rmnet_map_tx_aggregate_exit(struct rmnet_port *port);
void rmnet_map_update_ul_agg_config(struct rmnet_port *port, u32 size,
u32 count, u32 time);
+u32 rmnet_map_validate_packet_len(struct sk_buff *skb, struct rmnet_port *port);
#endif /* _RMNET_MAP_H_ */
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 8b4640c5d61e..305ae15ae8f3 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -333,54 +333,62 @@ struct rmnet_map_header *rmnet_map_add_map_header(struct sk_buff *skb,
return map_header;
}
-/* Deaggregates a single packet
- * A whole new buffer is allocated for each portion of an aggregated frame.
- * Caller should keep calling deaggregate() on the source skb until 0 is
- * returned, indicating that there are no more packets to deaggregate. Caller
- * is responsible for freeing the original skb.
- */
-struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
- struct rmnet_port *port)
+u32 rmnet_map_validate_packet_len(struct sk_buff *skb, struct rmnet_port *port)
{
struct rmnet_map_v5_csum_header *next_hdr = NULL;
struct rmnet_map_header *maph;
void *data = skb->data;
- struct sk_buff *skbn;
- u8 nexthdr_type;
u32 packet_len;
- if (skb->len == 0)
- return NULL;
+ if (skb->len < sizeof(*maph))
+ return 0;
maph = (struct rmnet_map_header *)skb->data;
+
+ /* Some hardware can send us empty frames. Catch them */
+ if (!maph->pkt_len)
+ return 0;
+
packet_len = ntohs(maph->pkt_len) + sizeof(*maph);
if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) {
packet_len += sizeof(struct rmnet_map_dl_csum_trailer);
- } else if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) {
- if (!(maph->flags & MAP_CMD_FLAG)) {
- packet_len += sizeof(*next_hdr);
- if (maph->flags & MAP_NEXT_HEADER_FLAG)
- next_hdr = data + sizeof(*maph);
- else
- /* Mapv5 data pkt without csum hdr is invalid */
- return NULL;
- }
+ } else if ((port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
+ !(maph->flags & MAP_CMD_FLAG)) {
+ /* Mapv5 data pkt without csum hdr is invalid */
+ if (!(maph->flags & MAP_NEXT_HEADER_FLAG))
+ return 0;
+
+ packet_len += sizeof(*next_hdr);
+ next_hdr = data + sizeof(*maph);
}
- if (((int)skb->len - (int)packet_len) < 0)
- return NULL;
+ if (skb->len < packet_len)
+ return 0;
- /* Some hardware can send us empty frames. Catch them */
- if (!maph->pkt_len)
- return NULL;
+ if (next_hdr &&
+ u8_get_bits(next_hdr->header_info, MAPV5_HDRINFO_HDR_TYPE_FMASK) !=
+ RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD)
+ return 0;
- if (next_hdr) {
- nexthdr_type = u8_get_bits(next_hdr->header_info,
- MAPV5_HDRINFO_HDR_TYPE_FMASK);
- if (nexthdr_type != RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD)
- return NULL;
- }
+ return packet_len;
+}
+
+/* Deaggregates a single packet
+ * A whole new buffer is allocated for each portion of an aggregated frame.
+ * Caller should keep calling deaggregate() on the source skb until 0 is
+ * returned, indicating that there are no more packets to deaggregate. Caller
+ * is responsible for freeing the original skb.
+ */
+struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
+ struct rmnet_port *port)
+{
+ struct sk_buff *skbn;
+ u32 packet_len;
+
+ packet_len = rmnet_map_validate_packet_len(skb, port);
+ if (!packet_len)
+ return NULL;
skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC);
if (!skbn)
--
2.43.0
^ permalink raw reply related
* Re: [PATCH net v2] net: qualcomm: rmnet: validate MAP frame length before ingress parsing
From: Xiang Mei @ 2026-06-30 17:43 UTC (permalink / raw)
To: subash.a.kasiviswanathan
Cc: sean.tranchetti, netdev, andrew+netdev, davem, edumazet, kuba,
pabeni, linux-kernel, bestswngs
In-Reply-To: <20260630174110.2003121-1-xmei5@asu.edu>
Thanks so much for your refactor. It's better! I took it for v2 with
two small changes:
- Dropped the goto err1/err0; the helper has no cleanup, so it just returns
0 or packet_len directly. (err0 was the success path anyway.)
- The no-agg caller leaks the skb when validation fails, since
__rmnet_map_ingress_handler() is the one that owns and frees it:
if (rmnet_map_validate_packet_len(skb, port))
__rmnet_map_ingress_handler(skb, port);
else
kfree_skb(skb);
I also bounds-check the MAP header up front (skb->len < sizeof(*maph)),
since the no-agg path can get a frame shorter than the header before
maph->pkt_len is read.
v2 sent with you as Suggested-by. Tested with the original repro: OOB gone
on the no-agg path, valid frames and the deagg path unaffected.
Xiang
On Tue, Jun 30, 2026 at 10:41 AM Xiang Mei <xmei5@asu.edu> wrote:
>
> When ingress deaggregation is disabled, rmnet_map_ingress_handler() passes
> the skb straight to __rmnet_map_ingress_handler(), skipping the length
> validation that rmnet_map_deaggregate() performs on the aggregated path.
> The parser then dereferences the MAP header and csum header/trailer based on
> the on-wire pkt_len without checking skb->len, so a short frame is read out
> of bounds:
>
> BUG: KASAN: slab-out-of-bounds in rmnet_map_checksum_downlink_packet
> Read of size 1 at addr ffff88801118ed00 by task exploit/147
> Call Trace:
> ...
> rmnet_map_checksum_downlink_packet (drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:413)
> __rmnet_map_ingress_handler (drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c:96)
> rmnet_rx_handler (drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c:129)
> __netif_receive_skb_core.constprop.0 (net/core/dev.c:6089)
> netif_receive_skb (net/core/dev.c:6460)
> tun_get_user (drivers/net/tun.c:1955)
> tun_chr_write_iter (drivers/net/tun.c:2001)
> vfs_write (fs/read_write.c:688)
> ksys_write (fs/read_write.c:740)
> do_syscall_64 (arch/x86/entry/syscall_64.c:94)
> ...
>
> Factor that validation out of rmnet_map_deaggregate() into
> rmnet_map_validate_packet_len() and run it on the no-aggregation path too.
> The MAP header is bounds-checked first, since this path can receive a frame
> shorter than the header.
>
> Fixes: ceed73a2cf4a ("drivers: net: ethernet: qualcomm: rmnet: Initial implementation")
> Reported-by: Weiming Shi <bestswngs@gmail.com>
> Suggested-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
> Signed-off-by: Xiang Mei <xmei5@asu.edu>
> ---
> v2: Validate on the no-aggregation path by reusing the deaggregation
> length checks (factored into rmnet_map_validate_packet_len()) instead
> of adding separate pskb_may_pull() guards in __rmnet_map_ingress_handler().
>
> .../ethernet/qualcomm/rmnet/rmnet_handlers.c | 5 +-
> .../net/ethernet/qualcomm/rmnet/rmnet_map.h | 1 +
> .../ethernet/qualcomm/rmnet/rmnet_map_data.c | 72 ++++++++++---------
> 3 files changed, 45 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> index 9f3479500f85..d055a2628d8c 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> @@ -126,7 +126,10 @@ rmnet_map_ingress_handler(struct sk_buff *skb,
>
> consume_skb(skb);
> } else {
> - __rmnet_map_ingress_handler(skb, port);
> + if (rmnet_map_validate_packet_len(skb, port))
> + __rmnet_map_ingress_handler(skb, port);
> + else
> + kfree_skb(skb);
> }
> }
>
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> index b70284095568..60ca8b780c88 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> @@ -59,5 +59,6 @@ void rmnet_map_tx_aggregate_init(struct rmnet_port *port);
> void rmnet_map_tx_aggregate_exit(struct rmnet_port *port);
> void rmnet_map_update_ul_agg_config(struct rmnet_port *port, u32 size,
> u32 count, u32 time);
> +u32 rmnet_map_validate_packet_len(struct sk_buff *skb, struct rmnet_port *port);
>
> #endif /* _RMNET_MAP_H_ */
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 8b4640c5d61e..305ae15ae8f3 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -333,54 +333,62 @@ struct rmnet_map_header *rmnet_map_add_map_header(struct sk_buff *skb,
> return map_header;
> }
>
> -/* Deaggregates a single packet
> - * A whole new buffer is allocated for each portion of an aggregated frame.
> - * Caller should keep calling deaggregate() on the source skb until 0 is
> - * returned, indicating that there are no more packets to deaggregate. Caller
> - * is responsible for freeing the original skb.
> - */
> -struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
> - struct rmnet_port *port)
> +u32 rmnet_map_validate_packet_len(struct sk_buff *skb, struct rmnet_port *port)
> {
> struct rmnet_map_v5_csum_header *next_hdr = NULL;
> struct rmnet_map_header *maph;
> void *data = skb->data;
> - struct sk_buff *skbn;
> - u8 nexthdr_type;
> u32 packet_len;
>
> - if (skb->len == 0)
> - return NULL;
> + if (skb->len < sizeof(*maph))
> + return 0;
>
> maph = (struct rmnet_map_header *)skb->data;
> +
> + /* Some hardware can send us empty frames. Catch them */
> + if (!maph->pkt_len)
> + return 0;
> +
> packet_len = ntohs(maph->pkt_len) + sizeof(*maph);
>
> if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) {
> packet_len += sizeof(struct rmnet_map_dl_csum_trailer);
> - } else if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) {
> - if (!(maph->flags & MAP_CMD_FLAG)) {
> - packet_len += sizeof(*next_hdr);
> - if (maph->flags & MAP_NEXT_HEADER_FLAG)
> - next_hdr = data + sizeof(*maph);
> - else
> - /* Mapv5 data pkt without csum hdr is invalid */
> - return NULL;
> - }
> + } else if ((port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
> + !(maph->flags & MAP_CMD_FLAG)) {
> + /* Mapv5 data pkt without csum hdr is invalid */
> + if (!(maph->flags & MAP_NEXT_HEADER_FLAG))
> + return 0;
> +
> + packet_len += sizeof(*next_hdr);
> + next_hdr = data + sizeof(*maph);
> }
>
> - if (((int)skb->len - (int)packet_len) < 0)
> - return NULL;
> + if (skb->len < packet_len)
> + return 0;
>
> - /* Some hardware can send us empty frames. Catch them */
> - if (!maph->pkt_len)
> - return NULL;
> + if (next_hdr &&
> + u8_get_bits(next_hdr->header_info, MAPV5_HDRINFO_HDR_TYPE_FMASK) !=
> + RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD)
> + return 0;
>
> - if (next_hdr) {
> - nexthdr_type = u8_get_bits(next_hdr->header_info,
> - MAPV5_HDRINFO_HDR_TYPE_FMASK);
> - if (nexthdr_type != RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD)
> - return NULL;
> - }
> + return packet_len;
> +}
> +
> +/* Deaggregates a single packet
> + * A whole new buffer is allocated for each portion of an aggregated frame.
> + * Caller should keep calling deaggregate() on the source skb until 0 is
> + * returned, indicating that there are no more packets to deaggregate. Caller
> + * is responsible for freeing the original skb.
> + */
> +struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
> + struct rmnet_port *port)
> +{
> + struct sk_buff *skbn;
> + u32 packet_len;
> +
> + packet_len = rmnet_map_validate_packet_len(skb, port);
> + if (!packet_len)
> + return NULL;
>
> skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC);
> if (!skbn)
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH v6 1/9] block: partitions: of: Skip child nodes without reg property
From: Rob Herring @ 2026-06-30 18:02 UTC (permalink / raw)
To: Loic Poulain
Cc: Ulf Hansson, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Jens Axboe, Johannes Berg, Jeff Johnson,
Bartosz Golaszewski, Marcel Holtmann, Luiz Augusto von Dentz,
Balakrishna Godavarthi, Rocky Liao, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Srinivas Kandagatla,
Andrew Lunn, Heiner Kallweit, Russell King, Saravana Kannan,
Christian Marangi, linux-mmc, devicetree, linux-kernel,
linux-arm-msm, linux-block, linux-wireless, ath10k,
linux-bluetooth, netdev, daniel, stable, Bartosz Golaszewski
In-Reply-To: <20260629-block-as-nvmem-v6-1-f02513dcd46d@oss.qualcomm.com>
On Mon, Jun 29, 2026 at 10:55:20AM +0200, Loic Poulain wrote:
> Child nodes of a fixed-partitions node are not necessarily partition
> entries, for example an nvmem-layout node has no reg property. The
> current code passes a NULL reg pointer and uninitialized len to the
> length check, which can result in a kernel panic or silent failure to
> register any partitions.
That does not sound right to me. A fixed-partitions node should only be
defining partitions with address ranges. I would expect a partition node
could be nvmem-layout, but not the whole address range. If you wanted
the latter, then just do:
partitions {
...
};
nvmem-layout {
...
};
Rob
^ permalink raw reply
* Re: [PATCH net] net/smc: fix UAF in smc_cdc_rx_handler() by pinning the socket
From: Xiang Mei @ 2026-06-30 18:18 UTC (permalink / raw)
To: Sidraya Jayagond
Cc: D . Wythe, Dust Li, Wenjia Zhang, Mahanta Jambigi, Tony Lu,
Wen Gu, netdev, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Hans Wippel, linux-rdma, linux-s390,
Weiming Shi
In-Reply-To: <cbd7c6c1-15e9-4a9a-aaca-4cbb5bd157c7@linux.ibm.com>
On Mon, Jun 29, 2026 at 10:40 PM Sidraya Jayagond <sidraya@linux.ibm.com> wrote:
>
>
>
> On 27/06/26 7:19 am, Xiang Mei wrote:
> > smc_cdc_rx_handler() looks up the connection by token under the link
> > group's conns_lock, drops the lock, and then dereferences conn and the
> > smc_sock derived from it, ending in sock_hold(&smc->sk) inside
> > smc_cdc_msg_recv(). No reference is held across the lock release.
> >
> > The only reference pinning the socket while the connection is
> > discoverable in the link group is taken in smc_lgr_register_conn()
> > (sock_hold) and dropped in __smc_lgr_unregister_conn() (sock_put), both
> > under conns_lock. Once the handler drops conns_lock, a concurrent
> > close() -> smc_release() -> smc_conn_free() -> smc_lgr_unregister_conn()
> > can drop that reference and free the smc_sock, so the handler's later
> > sock_hold() runs on freed memory:
> >
> > WARNING: lib/refcount.c:25 at refcount_warn_saturate
> > Workqueue: rxe_wq do_work
> > refcount_warn_saturate (lib/refcount.c:25)
> > smc_cdc_msg_recv (net/smc/smc_cdc.c:430)
> > smc_cdc_rx_handler (net/smc/smc_cdc.c:502)
> > smc_wr_rx_tasklet_fn (net/smc/smc_wr.c:445)
> > tasklet_action_common (kernel/softirq.c:938)
> > handle_softirqs (kernel/softirq.c:622)
> > Kernel panic - not syncing: panic_on_warn set
> >
> > Only SMC-R is affected. The SMC-D receive tasklet is stopped by
> > tasklet_kill(&conn->rx_tsklet) in smc_conn_free() before the connection
> > is unregistered, so it cannot run concurrently with the free.
> >
> > Take the socket reference while still holding conns_lock, so the
> > registration reference can no longer be the last one, and drop it once
> > the handler is done.
> >
> > Fixes: d7b0e37c1ac1 ("net/smc: restructure CDC message reception")
> > Reported-by: Weiming Shi <bestswngs@gmail.com>
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Xiang Mei <xmei5@asu.edu>
> > ---
> > net/smc/smc_cdc.c | 12 +++++++++---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> > index 619b3bab3824..b809139d7e87 100644
> > --- a/net/smc/smc_cdc.c
> > +++ b/net/smc/smc_cdc.c
> > @@ -483,21 +483,27 @@ static void smc_cdc_rx_handler(struct ib_wc *wc, void *buf)
> > lgr = smc_get_lgr(link);
> > read_lock_bh(&lgr->conns_lock);
> > conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
> > + if (conn && !conn->out_of_sync)
> > + sock_hold(&container_of(conn, struct smc_sock, conn)->sk);
> > + else
> > + conn = NULL;
> > read_unlock_bh(&lgr->conns_lock);
> > - if (!conn || conn->out_of_sync)
> > + if (!conn)
> > return;
> > smc = container_of(conn, struct smc_sock, conn);
> >
>
> Fix looks correct.
> A few nits on the implementation:
> container_of() is called twice for the same conn. The conn = NULL
> sentinel and the second post unlock check can also be dropped. Flip the
> condition, early return inside the lock, compute smc once:
>
> if (!conn || conn->out_of_sync) {
> read_unlock_bh(&lgr->conns_lock);
> return;
> }
> smc = container_of(conn, struct smc_sock, conn);
> sock_hold(&smc->sk);
> read_unlock_bh(&lgr->conns_lock);
>
> Also please initialize smc = NULL at declaration, it's not a bug now
> since the early return guards it, just to make it refactor safe.
>
Thanks so much for the review!
Both suggestions are good. v2 takes the reference under conns_lock and
returns early inside the lock. And smc is also initialized to NULL at
declaration.
Will send v2.
Xiang
> > if (cdc->prod_flags.failover_validation) {
> > smc_cdc_msg_validate(smc, cdc, link);
> > - return;
> > + goto out;
> > }
> > if (smc_cdc_before(ntohs(cdc->seqno),
> > conn->local_rx_ctrl.seqno))
> > /* received seqno is old */
> > - return;
> > + goto out;
> >
> > smc_cdc_msg_recv(smc, cdc);
> > +out:
> > + sock_put(&smc->sk);
> > }
> >
> > static struct smc_wr_rx_handler smc_cdc_rx_handlers[] = {
>
^ permalink raw reply
* Re: [PATCH net-next v2 1/4] net: add sockopt_init_user() for getsockopt conversion
From: Stanislav Fomichev @ 2026-06-30 18:19 UTC (permalink / raw)
To: Breno Leitao
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Willem de Bruijn, Shuah Khan, netdev, linux-kernel,
linux-kselftest, kernel-team
In-Reply-To: <20260630-getsockopt_phase2-v2-1-193335f3d4d1@debian.org>
On 06/30, Breno Leitao wrote:
> Add a helper that initializes a user-backed sockopt_t from the (optval,
> optlen) __user pair passed to a getsockopt() callback.
>
> It is used by transitional __user getsockopt wrappers while the
> proto-layer getsockopt callbacks are converted to take a sockopt_t, and
> is removed once the conversion is complete.
>
> The goal is to help to convert leafs. Example:
>
> sock_common_getsockopt(... char __user *optval, int __user *optlen)
> → udp_getsockopt(sk, level, optname, optval__user, optlen__user)
> → udp_lib_getsockopt(sk, level, optname, &opt) /* needs a sockopt_t */
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
^ permalink raw reply
* Re: [PATCH net-next v2 2/4] udp: convert udp_lib_getsockopt to sockopt_t
From: Stanislav Fomichev @ 2026-06-30 18:20 UTC (permalink / raw)
To: Breno Leitao
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Willem de Bruijn, Shuah Khan, netdev, linux-kernel,
linux-kselftest, kernel-team
In-Reply-To: <20260630-getsockopt_phase2-v2-2-193335f3d4d1@debian.org>
On 06/30, Breno Leitao wrote:
> In preparation for converting the proto-layer getsockopt callbacks to the
> sockopt_t interface, switch udp_lib_getsockopt() to take a sockopt_t.
>
> The thin udp_getsockopt()/udpv6_getsockopt() wrappers keep their __user
> signature for now: they build a user-backed sockopt_t with
> sockopt_init_user(), call the helper, and write the returned length back
> to optlen. The helper uses copy_to_iter() instead of copy_to_user().
> No functional change.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
^ permalink raw reply
* Re: [PATCH net-next v2 3/4] ipv4: raw: convert do_raw_getsockopt to sockopt_t
From: Stanislav Fomichev @ 2026-06-30 18:20 UTC (permalink / raw)
To: Breno Leitao
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Willem de Bruijn, Shuah Khan, netdev, linux-kernel,
linux-kselftest, kernel-team
In-Reply-To: <20260630-getsockopt_phase2-v2-3-193335f3d4d1@debian.org>
On 06/30, Breno Leitao wrote:
> Continue converting the proto-layer getsockopt callbacks to the sockopt_t
> interface, switching do_raw_getsockopt() and its raw_geticmpfilter()
> helper to take a sockopt_t.
>
> The thin raw_getsockopt() wrapper keeps its __user signature for now: it
> builds a user-backed sockopt_t with sockopt_init_user(), calls the helper,
> and writes the returned length back to optlen. The helper uses
> copy_to_iter() instead of copy_to_user(). No functional change.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
^ permalink raw reply
* Re: [PATCH net-next v2 4/4] selftests: net: getsockopt_iter: add raw ICMP_FILTER coverage
From: Stanislav Fomichev @ 2026-06-30 18:20 UTC (permalink / raw)
To: Breno Leitao
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Willem de Bruijn, Shuah Khan, netdev, linux-kernel,
linux-kselftest, kernel-team
In-Reply-To: <20260630-getsockopt_phase2-v2-4-193335f3d4d1@debian.org>
On 06/30, Breno Leitao wrote:
> Exercise the raw getsockopt path now backed by sockopt_t. ICMP_FILTER
> returns a fixed-size struct and, unlike the int/u64 options already
> covered, clamps the length down to the user buffer on a short read
> instead of failing, so check that semantic explicitly along with the
> exact and oversized cases, the -EOPNOTSUPP path on a non-ICMP raw
> socket, and an unknown optname.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
^ permalink raw reply
* [PATCH net-next 0/6] net: hold instance lock around NETDEV_DOWN and NETDEV_GOING_DOWN
From: Stanislav Fomichev @ 2026-06-30 18:21 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni
NETDEV_UP and NETDEV_REGISTER already run under the per-device
instance lock. The teardown side does not. Make it symmetric so
ops-locked drivers can rely on the lock being held in both
directions.
Stanislav Fomichev (6):
net: hold instance lock around NETDEV_DOWN/GOING_DOWN
net: dsa: hold instance lock on close-on-shutdown paths
net: mtk_eth_soc: hold instance lock around DMA-device-swap close
net: rtnetlink: take instance lock inside rtnl_configure_link
net: require instance lock for NETDEV_DOWN/GOING_DOWN notifiers
net: document NETDEV_UNREGISTER unlocked rationale
Documentation/networking/netdevices.rst | 10 ++++++++++
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 5 +++++
net/core/dev.c | 5 +++++
net/core/lock_debug.c | 4 ++--
net/core/rtnetlink.c | 17 ++++++++++-------
net/dsa/dsa.c | 20 +++++++++++++++++---
net/dsa/user.c | 19 +++++++++++++++++--
7 files changed, 66 insertions(+), 14 deletions(-)
--
2.53.0-Meta
^ permalink raw reply
* [PATCH net-next 1/6] net: hold instance lock around NETDEV_DOWN/GOING_DOWN
From: Stanislav Fomichev @ 2026-06-30 18:21 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni
In-Reply-To: <20260630182129.1601784-1-sdf@fomichev.me>
Mirror what call_netdevice_register_net_notifiers does but for the
teardown. Cover only DOWN and GOING_DOWN. UNREGISTER is still unlocked
because of the SW devices using dev_xxx methods.
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
net/core/dev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/core/dev.c b/net/core/dev.c
index 4b3d5cfdf6e0..9d49493f4fb5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1912,9 +1912,11 @@ static void call_netdevice_unregister_notifiers(struct notifier_block *nb,
struct net_device *dev)
{
if (dev->flags & IFF_UP) {
+ netdev_lock_ops(dev);
call_netdevice_notifier(nb, NETDEV_GOING_DOWN,
dev);
call_netdevice_notifier(nb, NETDEV_DOWN, dev);
+ netdev_unlock_ops(dev);
}
call_netdevice_notifier(nb, NETDEV_UNREGISTER, dev);
}
--
2.53.0-Meta
^ permalink raw reply related
* [PATCH net-next 2/6] net: dsa: hold instance lock on close-on-shutdown paths
From: Stanislav Fomichev @ 2026-06-30 18:21 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni
In-Reply-To: <20260630182129.1601784-1-sdf@fomichev.me>
netif_close_many will soon assert ops lock (for locked DOWN/GOING_DOWN).
Update dsa_switch_shutdown to manually grab and release the ops lock.
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
net/dsa/dsa.c | 20 +++++++++++++++++---
net/dsa/user.c | 19 +++++++++++++++++--
2 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 9cb732f6b1e3..da53a666d4b8 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -18,6 +18,7 @@
#include <linux/of.h>
#include <linux/of_net.h>
#include <net/dsa_stubs.h>
+#include <net/netdev_lock.h>
#include <net/sch_generic.h>
#include "conduit.h"
@@ -1620,10 +1621,23 @@ void dsa_switch_shutdown(struct dsa_switch *ds)
rtnl_lock();
- dsa_switch_for_each_cpu_port(dp, ds)
- list_add(&dp->conduit->close_list, &close_list);
+ dsa_switch_for_each_cpu_port(dp, ds) {
+ if (!(dp->conduit->flags & IFF_UP))
+ continue;
+ list_add_tail(&dp->conduit->close_list, &close_list);
+ netdev_lock_ops(dp->conduit);
+ }
+
+ netif_close_many(&close_list, false);
- netif_close_many(&close_list, true);
+ while (!list_empty(&close_list)) {
+ struct net_device *conduit;
+
+ conduit = list_first_entry(&close_list, struct net_device,
+ close_list);
+ netdev_unlock_ops(conduit);
+ list_del_init(&conduit->close_list);
+ }
dsa_switch_for_each_user_port(dp, ds) {
conduit = dsa_port_to_conduit(dp);
diff --git a/net/dsa/user.c b/net/dsa/user.c
index 8704c1a3a5b7..8ea47444d6d5 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -13,6 +13,7 @@
#include <linux/of_net.h>
#include <linux/of_mdio.h>
#include <linux/mdio.h>
+#include <net/netdev_lock.h>
#include <net/rtnetlink.h>
#include <net/pkt_cls.h>
#include <net/selftests.h>
@@ -3600,10 +3601,24 @@ static int dsa_user_netdevice_event(struct notifier_block *nb,
if (dp->cpu_dp != cpu_dp)
continue;
- list_add(&dp->user->close_list, &close_list);
+ if (!(dp->user->flags & IFF_UP))
+ continue;
+
+ list_add_tail(&dp->user->close_list, &close_list);
+ netdev_lock_ops(dp->user);
}
- netif_close_many(&close_list, true);
+ netif_close_many(&close_list, false);
+
+ while (!list_empty(&close_list)) {
+ struct net_device *user_dev;
+
+ user_dev = list_first_entry(&close_list,
+ struct net_device,
+ close_list);
+ netdev_unlock_ops(user_dev);
+ list_del_init(&user_dev->close_list);
+ }
return NOTIFY_OK;
}
--
2.53.0-Meta
^ permalink raw reply related
* [PATCH net-next 3/6] net: mtk_eth_soc: hold instance lock around DMA-device-swap close
From: Stanislav Fomichev @ 2026-06-30 18:21 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni
In-Reply-To: <20260630182129.1601784-1-sdf@fomichev.me>
netif_close_many will soon assert ops lock (for locked DOWN/GOING_DOWN).
Update mtk_eth_set_dma_device to manually grab and release the ops lock.
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 5d291e50a47b..fe7610c42e5d 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -26,6 +26,7 @@
#include <linux/bitfield.h>
#include <net/dsa.h>
#include <net/dst_metadata.h>
+#include <net/netdev_lock.h>
#include <net/page_pool/helpers.h>
#include <linux/genalloc.h>
@@ -5030,10 +5031,14 @@ void mtk_eth_set_dma_device(struct mtk_eth *eth, struct device *dma_dev)
continue;
list_add_tail(&dev->close_list, &dev_list);
+ netdev_lock_ops(dev);
}
netif_close_many(&dev_list, false);
+ list_for_each_entry(dev, &dev_list, close_list)
+ netdev_unlock_ops(dev);
+
eth->dma_dev = dma_dev;
list_for_each_entry_safe(dev, tmp, &dev_list, close_list) {
--
2.53.0-Meta
^ permalink raw reply related
* [PATCH net-next 4/6] net: rtnetlink: take instance lock inside rtnl_configure_link
From: Stanislav Fomichev @ 2026-06-30 18:21 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni
In-Reply-To: <20260630182129.1601784-1-sdf@fomichev.me>
rtnl_configure_link calls __dev_change_flags() and __dev_notify_flags,
both need the instance lock. rtnl_newlink_create grabs it but stacked
devices do not. Move the lock inside rtnl_configure_link.
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
net/core/rtnetlink.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 12aa3aa1688b..1b7d6f6b8b68 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3660,14 +3660,16 @@ int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm,
u32 portid, const struct nlmsghdr *nlh)
{
unsigned int old_flags, changed;
- int err;
+ int err = 0;
+
+ netdev_lock_ops(dev);
old_flags = dev->flags;
if (ifm && (ifm->ifi_flags || ifm->ifi_change)) {
err = __dev_change_flags(dev, rtnl_dev_combine_flags(dev, ifm),
NULL);
if (err < 0)
- return err;
+ goto out;
}
changed = old_flags ^ dev->flags;
@@ -3677,7 +3679,10 @@ int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm,
}
__dev_notify_flags(dev, old_flags, changed, portid, nlh);
- return 0;
+
+out:
+ netdev_unlock_ops(dev);
+ return err;
}
EXPORT_SYMBOL(rtnl_configure_link);
@@ -3918,22 +3923,20 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
goto out;
}
- netdev_lock_ops(dev);
-
err = rtnl_configure_link(dev, ifm, portid, nlh);
if (err < 0)
goto out_unregister;
if (tb[IFLA_MASTER]) {
+ netdev_lock_ops(dev);
err = do_set_master(dev, nla_get_u32(tb[IFLA_MASTER]), extack);
+ netdev_unlock_ops(dev);
if (err)
goto out_unregister;
}
- netdev_unlock_ops(dev);
out:
return err;
out_unregister:
- netdev_unlock_ops(dev);
if (ops->newlink) {
LIST_HEAD(list_kill);
--
2.53.0-Meta
^ permalink raw reply related
* [PATCH net-next 5/6] net: require instance lock for NETDEV_DOWN/GOING_DOWN notifiers
From: Stanislav Fomichev @ 2026-06-30 18:21 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni
In-Reply-To: <20260630182129.1601784-1-sdf@fomichev.me>
Sprinkle a few asserts about ops lock: netif_close_many and __dev_notify_flags
should now consistently run under the lock
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
Documentation/networking/netdevices.rst | 2 ++
net/core/dev.c | 3 +++
net/core/lock_debug.c | 4 ++--
3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/Documentation/networking/netdevices.rst b/Documentation/networking/netdevices.rst
index d2a238f8cc8b..1bb68a73bb67 100644
--- a/Documentation/networking/netdevices.rst
+++ b/Documentation/networking/netdevices.rst
@@ -421,6 +421,8 @@ For devices with locked ops, currently only the following notifiers are
* ``NETDEV_CHANGENAME``
* ``NETDEV_REGISTER``
* ``NETDEV_UP``
+* ``NETDEV_DOWN``
+* ``NETDEV_GOING_DOWN``
The following notifiers are running without the lock:
* ``NETDEV_UNREGISTER``
diff --git a/net/core/dev.c b/net/core/dev.c
index 9d49493f4fb5..714d05283500 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1802,6 +1802,7 @@ void netif_close_many(struct list_head *head, bool unlink)
__dev_close_many(head);
list_for_each_entry_safe(dev, tmp, head, close_list) {
+ netdev_assert_locked_ops_compat(dev);
rtmsg_ifinfo(RTM_NEWLINK, dev, IFF_UP | IFF_RUNNING, GFP_KERNEL, 0, NULL);
call_netdevice_notifiers(NETDEV_DOWN, dev);
if (unlink)
@@ -9787,6 +9788,8 @@ void __dev_notify_flags(struct net_device *dev, unsigned int old_flags,
{
unsigned int changes = dev->flags ^ old_flags;
+ netdev_assert_locked_ops_compat(dev);
+
if (gchanges)
rtmsg_ifinfo(RTM_NEWLINK, dev, gchanges, GFP_ATOMIC, portid, nlh);
diff --git a/net/core/lock_debug.c b/net/core/lock_debug.c
index 8a81c5430705..abc4c00728b1 100644
--- a/net/core/lock_debug.c
+++ b/net/core/lock_debug.c
@@ -24,15 +24,15 @@ int netdev_debug_event(struct notifier_block *nb, unsigned long event,
case NETDEV_CHANGE:
case NETDEV_REGISTER:
case NETDEV_UP:
+ case NETDEV_DOWN:
+ case NETDEV_GOING_DOWN:
netdev_assert_locked_ops_compat(dev);
fallthrough;
- case NETDEV_DOWN:
case NETDEV_REBOOT:
case NETDEV_UNREGISTER:
case NETDEV_CHANGEMTU:
case NETDEV_CHANGEADDR:
case NETDEV_PRE_CHANGEADDR:
- case NETDEV_GOING_DOWN:
case NETDEV_FEAT_CHANGE:
case NETDEV_BONDING_FAILOVER:
case NETDEV_PRE_UP:
--
2.53.0-Meta
^ permalink raw reply related
* [PATCH net-next 6/6] net: document NETDEV_UNREGISTER unlocked rationale
From: Stanislav Fomichev @ 2026-06-30 18:21 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni
In-Reply-To: <20260630182129.1601784-1-sdf@fomichev.me>
The lock-state table marks UNREGISTER as unlocked without saying
why. Add a short note that many handlers release the lowers via
dev_close().
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
Documentation/networking/netdevices.rst | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/networking/netdevices.rst b/Documentation/networking/netdevices.rst
index 1bb68a73bb67..761cdb08bf0f 100644
--- a/Documentation/networking/netdevices.rst
+++ b/Documentation/networking/netdevices.rst
@@ -427,6 +427,14 @@ For devices with locked ops, currently only the following notifiers are
The following notifiers are running without the lock:
* ``NETDEV_UNREGISTER``
+Many ``NETDEV_UNREGISTER`` handlers release their lowers with
+``dev_close()``, which takes the instance lock itself. Holding
+the lock across UNREGISTER would deadlock.
+
+Moving UNREGISTER under the lock is mechanical: switch those
+callers to the ``netif_*()`` lock-held variants. Deferred to
+limit churn.
+
There are no clear expectations for the remaining notifiers. Notifiers not on
the list may run with or without the instance lock, potentially even invoking
the same notifier type with and without the lock from different code paths.
--
2.53.0-Meta
^ permalink raw reply related
* Re: [PATCH net-next v5 5/5] ionic: Add .get_fec_stats ethtool handler
From: Eric Joyner @ 2026-06-30 18:24 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, Brett Creeley, Andrew Lunn, David S. Miller, Eric Dumazet,
Paolo Abeni, Nikhil P . Rao, Simon Horman
In-Reply-To: <20260615182732.7d28e31a@kernel.org>
On 6/15/2026 6:27 PM, Jakub Kicinski wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Sun, 14 Jun 2026 13:53:03 -0700 Eric Joyner wrote:
>> + if (fec_cw_err_bin != IONIC_STAT_INVALID)
>> + hist->values[i].sum = le64_to_cpu(fec_cw_err_bin);
>> + else
>> + hist->values[i].sum = 0;
>
> Setting the sum to zero is very much against the API contract with
> ethtool, no? Since we are just digging ourselves out of a whole with
> the link down events maybe let's be more strict going forward. :S
>
> I think mlx5 had a similar problem of not knowing bucket count.
> You can put the bucket table in some driver struct and populate it
> at runtime.
>
> Looking at this scheme, tho, I think the ethtool core is buggy for
> mlx5 :/ We should make a copy of the hist table, because we write
> the Netlink attrs after releasing the locks. Another call may start
> already and make the driver write its table in parallel.
>
> Please send a fix for that if you can, or LMK if not, I'll chase
> one of the people involved in adding the fec stats.
>
> I'll apply the first 4 patches here in the meantime.
Yeah, I'll work on a fix.
(Just to say that I didn't forget about this!)
- Eric
^ permalink raw reply
* [PATCH net v2] net/smc: fix UAF in smc_cdc_rx_handler() by pinning the socket
From: Xiang Mei @ 2026-06-30 18:32 UTC (permalink / raw)
To: Sidraya Jayagond, D . Wythe, Dust Li, Wenjia Zhang,
Mahanta Jambigi, Tony Lu, Wen Gu, netdev
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Hans Wippel, linux-rdma, linux-s390, Weiming Shi,
Xiang Mei
smc_cdc_rx_handler() looks up the connection by token under the link
group's conns_lock, drops the lock, and then dereferences conn and the
smc_sock derived from it, ending in sock_hold(&smc->sk) inside
smc_cdc_msg_recv(). No reference is held across the lock release.
The only reference pinning the socket while the connection is
discoverable in the link group is taken in smc_lgr_register_conn()
(sock_hold) and dropped in __smc_lgr_unregister_conn() (sock_put), both
under conns_lock. Once the handler drops conns_lock, a concurrent
close() -> smc_release() -> smc_conn_free() -> smc_lgr_unregister_conn()
can drop that reference and free the smc_sock, so the handler's later
sock_hold() runs on freed memory:
WARNING: lib/refcount.c:25 at refcount_warn_saturate
Workqueue: rxe_wq do_work
refcount_warn_saturate (lib/refcount.c:25)
smc_cdc_msg_recv (net/smc/smc_cdc.c:430)
smc_cdc_rx_handler (net/smc/smc_cdc.c:502)
smc_wr_rx_tasklet_fn (net/smc/smc_wr.c:445)
tasklet_action_common (kernel/softirq.c:938)
handle_softirqs (kernel/softirq.c:622)
Kernel panic - not syncing: panic_on_warn set
Only SMC-R is affected. The SMC-D receive tasklet is stopped by
tasklet_kill(&conn->rx_tsklet) in smc_conn_free() before the connection
is unregistered, so it cannot run concurrently with the free.
Take the socket reference while still holding conns_lock, so the
registration reference can no longer be the last one, and drop it once
the handler is done.
Fixes: d7b0e37c1ac1 ("net/smc: restructure CDC message reception")
Reported-by: Weiming Shi <bestswngs@gmail.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Xiang Mei <xmei5@asu.edu>
---
v2:
- Take the reference under conns_lock, and compute smc once
- Initialize smc = NULL at declaration
net/smc/smc_cdc.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 619b3bab3824..32d6d03df321 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -470,9 +470,9 @@ static void smc_cdc_rx_handler(struct ib_wc *wc, void *buf)
{
struct smc_link *link = (struct smc_link *)wc->qp->qp_context;
struct smc_cdc_msg *cdc = buf;
+ struct smc_sock *smc = NULL;
struct smc_connection *conn;
struct smc_link_group *lgr;
- struct smc_sock *smc;
if (wc->byte_len < offsetof(struct smc_cdc_msg, reserved))
return; /* short message */
@@ -483,21 +483,26 @@ static void smc_cdc_rx_handler(struct ib_wc *wc, void *buf)
lgr = smc_get_lgr(link);
read_lock_bh(&lgr->conns_lock);
conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
- read_unlock_bh(&lgr->conns_lock);
- if (!conn || conn->out_of_sync)
+ if (!conn || conn->out_of_sync) {
+ read_unlock_bh(&lgr->conns_lock);
return;
+ }
smc = container_of(conn, struct smc_sock, conn);
+ sock_hold(&smc->sk);
+ read_unlock_bh(&lgr->conns_lock);
if (cdc->prod_flags.failover_validation) {
smc_cdc_msg_validate(smc, cdc, link);
- return;
+ goto out;
}
if (smc_cdc_before(ntohs(cdc->seqno),
conn->local_rx_ctrl.seqno))
/* received seqno is old */
- return;
+ goto out;
smc_cdc_msg_recv(smc, cdc);
+out:
+ sock_put(&smc->sk);
}
static struct smc_wr_rx_handler smc_cdc_rx_handlers[] = {
--
2.43.0
^ permalink raw reply related
* [PATCH net-next v3 0/2] net/sched: sch_fq_pie: add per-flow class statistics
From: Hemendra M. Naik @ 2026-06-30 18:37 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, horms, jiri, jhs, shuah,
linux-kernel, linux-kselftest, vishy0777, tahiliani,
Hemendra M. Naik
FQ-PIE runs an independent PIE controller per flow but exposes no
per-flow statistics. This series wires up fq_pie_class_ops to expose
per-flow AQM state (prob, delay, deficit, avg_dq_rate)
via 'tc -s class show', following a similar pattern as FQ-CoDel.
---
Changelog:
v3:
- No changes since v2.
- Resending as the previous submission was deferred when the net-next tree closed during review.
- Corresponding iproute2 patch updated in response to review comments; no changes required for this patch.
v2:
- Addressed ABI backward compatibility issue for tc_fq_pie_xstats. (https://lore.kernel.org/netdev/20260614125000.6058-1-hemendranaik@gmail.com/)
v1:
- https://lore.kernel.org/netdev/20260531125314.22492-1-hemendranaik@gmail.com/
Hemendra M. Naik (2):
net/sched: sch_fq_pie: add per-flow statistics via class ops
selftests: tc-testing: add fq_pie per-flow class stats test
include/uapi/linux/pkt_sched.h | 29 ++++-
net/sched/sch_fq_pie.c | 118 +++++++++++++++++-
tools/include/uapi/linux/pkt_sched.h | 4 +-
.../tc-testing/tc-tests/qdiscs/fq_pie.json | 22 ++++
4 files changed, 163 insertions(+), 10 deletions(-)
--
2.34.1
^ permalink raw reply
* [PATCH net-next v3 1/2] net/sched: sch_fq_pie: add per-flow statistics via class ops
From: Hemendra M. Naik @ 2026-06-30 18:37 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, horms, jiri, jhs, shuah,
linux-kernel, linux-kselftest, vishy0777, tahiliani,
Hemendra M. Naik
In-Reply-To: <20260630183702.170798-1-hemendranaik@gmail.com>
FQ-PIE schedules independent PIE controllers per flow but exposes no
per-flow AQM state. Without class-level statistics there is no way to
observe the per-flow drop probability, queue delay, deficit or
dequeue rate from userspace.
Extend tc_fq_pie_xstats to support both qdisc and class-level
extended statistics.
- Add enum with QDISC and CLASS type discriminators.
- Add struct tc_fq_pie_cl_stats for per-flow metrics (prob,
delay, deficit, avg_dq_rate, dq_rate_estimating).
- Add empty struct tc_fq_pie_xqd_stats placeholder.
Wire up fq_pie_class_ops (.walk, .dump, .dump_stats) so that
'tc -s class show' against an fq_pie qdisc reports per-flow state:
prob per-flow PIE drop probability
delay per-flow queue sojourn time (microseconds)
deficit remaining DRR byte credits (signed integer)
avg_dq_rate dequeue rate estimate in bytes/second
(dq_rate_estimator mode only)
dq_rate_estimating flag indicating active delay estimation mode
Fix the 'delay' field comment in struct tc_pie_xstats from "in ms" to
"in microseconds" to match the kernel's
PSCHED_TICKS2NS / NSEC_PER_USEC conversion.
Also correct the avg_dq_rate comment in tc_pie_xstats from
"bits/pie_time" to "bytes/second" to match the actual kernel
conversion (avg_dq_rate * PSCHED_TICKS_PER_SEC >> PIE_SCALE).
Signed-off-by: Hemendra M. Naik <hemendranaik@gmail.com>
Signed-off-by: Vishal Kamath <vishy0777@gmail.com>
Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>
---
Changelog:
v3:
- No changes since v2.
- Resending as the previous submission was deferred when the net-next tree closed during review.
- Corresponding iproute2 patch updated in response to review comments; no changes required for this patch.
v2:
- Addressed ABI backward compatibility issue for tc_fq_pie_xstats. (https://lore.kernel.org/netdev/20260614125000.6058-2-hemendranaik@gmail.com/)
v1:
- https://lore.kernel.org/netdev/20260531125314.22492-2-hemendranaik@gmail.com
---
include/uapi/linux/pkt_sched.h | 29 ++++++-
net/sched/sch_fq_pie.c | 118 +++++++++++++++++++++++++--
tools/include/uapi/linux/pkt_sched.h | 4 +-
3 files changed, 141 insertions(+), 10 deletions(-)
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 490efd288526..b18f274b2ec5 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -920,9 +920,9 @@ enum {
struct tc_pie_xstats {
__u64 prob; /* current probability */
- __u32 delay; /* current delay in ms */
+ __u32 delay; /* current delay in microseconds */
__u32 avg_dq_rate; /* current average dq_rate in
- * bits/pie_time
+ * bytes/second
*/
__u32 dq_rate_estimating; /* is avg_dq_rate being calculated? */
__u32 packets_in; /* total number of packets enqueued */
@@ -953,6 +953,25 @@ enum {
};
#define TCA_FQ_PIE_MAX (__TCA_FQ_PIE_MAX - 1)
+enum {
+ TCA_FQ_PIE_XSTATS_QDISC,
+ TCA_FQ_PIE_XSTATS_CLASS,
+};
+
+struct tc_fq_pie_cl_stats {
+ __u64 prob; /* current probability */
+ __u32 delay; /* current delay in microseconds */
+ __s32 deficit; /* number of remaining byte credits */
+ __u32 avg_dq_rate; /* current average dq_rate in
+ * bytes/second
+ */
+ __u32 dq_rate_estimating; /* is avg_dq_rate being calculated? */
+};
+
+struct tc_fq_pie_xqd_stats {
+ /* placeholder for new qdisc-level stats */
+};
+
struct tc_fq_pie_xstats {
__u32 packets_in; /* total number of packets enqueued */
__u32 dropped; /* packets dropped due to fq_pie_action */
@@ -963,6 +982,12 @@ struct tc_fq_pie_xstats {
__u32 new_flows_len; /* count of flows in new list */
__u32 old_flows_len; /* count of flows in old list */
__u32 memory_usage; /* total memory across all queues */
+ __u32 type;
+ union {
+ struct tc_fq_pie_cl_stats class_stats;
+ struct tc_fq_pie_xqd_stats xqdisc_stats;
+ };
+
};
/* CBS */
diff --git a/net/sched/sch_fq_pie.c b/net/sched/sch_fq_pie.c
index 72f48fa4010b..60e85c002ae7 100644
--- a/net/sched/sch_fq_pie.c
+++ b/net/sched/sch_fq_pie.c
@@ -330,7 +330,7 @@ static int fq_pie_change(struct Qdisc *sch, struct nlattr *opt,
/* tupdate is in jiffies */
if (tb[TCA_FQ_PIE_TUPDATE])
WRITE_ONCE(q->p_params.tupdate,
- usecs_to_jiffies(nla_get_u32(tb[TCA_FQ_PIE_TUPDATE])));
+ usecs_to_jiffies(nla_get_u32(tb[TCA_FQ_PIE_TUPDATE])));
if (tb[TCA_FQ_PIE_ALPHA])
WRITE_ONCE(q->p_params.alpha,
@@ -509,7 +509,9 @@ static int fq_pie_dump(struct Qdisc *sch, struct sk_buff *skb)
static int fq_pie_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
{
struct fq_pie_sched_data *q = qdisc_priv(sch);
- struct tc_fq_pie_xstats st = { 0 };
+ struct tc_fq_pie_xstats st = {
+ .type = TCA_FQ_PIE_XSTATS_QDISC,
+ };
struct list_head *pos;
sch_tree_lock(sch);
@@ -517,10 +519,10 @@ static int fq_pie_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
st.packets_in = q->stats.packets_in;
st.overlimit = q->stats.overlimit;
st.overmemory = q->overmemory;
- st.dropped = q->stats.dropped;
- st.ecn_mark = q->stats.ecn_mark;
- st.new_flow_count = q->new_flow_count;
- st.memory_usage = q->memory_usage;
+ st.dropped = q->stats.dropped;
+ st.ecn_mark = q->stats.ecn_mark;
+ st.new_flow_count = q->new_flow_count;
+ st.memory_usage = q->memory_usage;
list_for_each(pos, &q->new_flows)
st.new_flows_len++;
@@ -561,7 +563,111 @@ static void fq_pie_destroy(struct Qdisc *sch)
kvfree(q->flows);
}
+static struct Qdisc *fq_pie_leaf(struct Qdisc *sch, unsigned long arg)
+{
+ return NULL;
+}
+
+static unsigned long fq_pie_find(struct Qdisc *sch, u32 classid)
+{
+ return 0;
+}
+
+static unsigned long fq_pie_bind(struct Qdisc *sch, unsigned long parent,
+ u32 classid)
+{
+ return 0;
+}
+
+static void fq_pie_unbind(struct Qdisc *q, unsigned long cl)
+{
+}
+
+static struct tcf_block *fq_pie_tcf_block(struct Qdisc *sch, unsigned long cl,
+ struct netlink_ext_ack *extack)
+{
+ struct fq_pie_sched_data *q = qdisc_priv(sch);
+
+ if (cl)
+ return NULL;
+ return q->block;
+}
+
+static int fq_pie_dump_class(struct Qdisc *sch, unsigned long cl,
+ struct sk_buff *skb, struct tcmsg *tcm)
+{
+ tcm->tcm_handle |= TC_H_MIN(cl);
+ return 0;
+}
+
+static int fq_pie_dump_class_stats(struct Qdisc *sch, unsigned long cl,
+ struct gnet_dump *d)
+{
+ struct fq_pie_sched_data *q = qdisc_priv(sch);
+ struct gnet_stats_queue qs = { 0 };
+ struct tc_fq_pie_xstats xstats;
+ u32 idx = cl - 1;
+
+ if (idx < q->flows_cnt) {
+ const struct fq_pie_flow *flow = &q->flows[idx];
+
+ memset(&xstats, 0, sizeof(xstats));
+ xstats.type = TCA_FQ_PIE_XSTATS_CLASS;
+ xstats.class_stats.prob = READ_ONCE(flow->vars.prob) << BITS_PER_BYTE;
+ xstats.class_stats.delay =
+ ((u32)PSCHED_TICKS2NS(READ_ONCE(flow->vars.qdelay))) /
+ NSEC_PER_USEC;
+ xstats.class_stats.deficit = READ_ONCE(flow->deficit);
+ xstats.class_stats.dq_rate_estimating =
+ READ_ONCE(q->p_params.dq_rate_estimator);
+
+ if (xstats.class_stats.dq_rate_estimating) {
+ xstats.class_stats.avg_dq_rate =
+ READ_ONCE(flow->vars.avg_dq_rate) *
+ (PSCHED_TICKS_PER_SEC) >> PIE_SCALE;
+ }
+
+ qs.qlen = READ_ONCE(flow->qlen);
+ qs.backlog = READ_ONCE(flow->backlog);
+ }
+ if (gnet_stats_copy_queue(d, NULL, &qs, qs.qlen) < 0)
+ return -1;
+ if (idx < q->flows_cnt)
+ return gnet_stats_copy_app(d, &xstats, sizeof(xstats));
+ return 0;
+}
+
+static void fq_pie_walk(struct Qdisc *sch, struct qdisc_walker *arg)
+{
+ struct fq_pie_sched_data *q = qdisc_priv(sch);
+ unsigned int i;
+
+ if (arg->stop)
+ return;
+
+ for (i = 0; i < q->flows_cnt; i++) {
+ if (list_empty(&q->flows[i].flowchain)) {
+ arg->count++;
+ continue;
+ }
+ if (!tc_qdisc_stats_dump(sch, i + 1, arg))
+ break;
+ }
+}
+
+static const struct Qdisc_class_ops fq_pie_class_ops = {
+ .leaf = fq_pie_leaf,
+ .find = fq_pie_find,
+ .tcf_block = fq_pie_tcf_block,
+ .bind_tcf = fq_pie_bind,
+ .unbind_tcf = fq_pie_unbind,
+ .dump = fq_pie_dump_class,
+ .dump_stats = fq_pie_dump_class_stats,
+ .walk = fq_pie_walk,
+};
+
static struct Qdisc_ops fq_pie_qdisc_ops __read_mostly = {
+ .cl_ops = &fq_pie_class_ops,
.id = "fq_pie",
.priv_size = sizeof(struct fq_pie_sched_data),
.enqueue = fq_pie_qdisc_enqueue,
diff --git a/tools/include/uapi/linux/pkt_sched.h b/tools/include/uapi/linux/pkt_sched.h
index 587481a19433..45ea10026742 100644
--- a/tools/include/uapi/linux/pkt_sched.h
+++ b/tools/include/uapi/linux/pkt_sched.h
@@ -847,8 +847,8 @@ enum {
struct tc_pie_xstats {
__u32 prob; /* current probability */
- __u32 delay; /* current delay in ms */
- __u32 avg_dq_rate; /* current average dq_rate in bits/pie_time */
+ __u32 delay; /* current delay in micoseconds */
+ __u32 avg_dq_rate; /* current average dq_rate in bytes/second */
__u32 packets_in; /* total number of packets enqueued */
__u32 dropped; /* packets dropped due to pie_action */
__u32 overlimit; /* dropped due to lack of space in queue */
--
2.34.1
^ permalink raw reply related
* [PATCH net-next v3 2/2] selftests: tc-testing: add fq_pie per-flow class stats test
From: Hemendra M. Naik @ 2026-06-30 18:37 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, horms, jiri, jhs, shuah,
linux-kernel, linux-kselftest, vishy0777, tahiliani,
Hemendra M. Naik
In-Reply-To: <20260630183702.170798-1-hemendranaik@gmail.com>
Add a tc-testing entry (id: 83c0) to verify the fq_pie class ops
wired up in the previous patch do not crash and integrate cleanly
with the tc class show path.
The test creates an fq_pie root qdisc on a dummy interface and runs
'tc -s class show'.
Signed-off-by: Hemendra M. Naik <hemendranaik@gmail.com>
Signed-off-by: Vishal Kamath <vishy0777@gmail.com>
Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>
---
Changelog:
v3:
- No changes since v2.
- Resending as the previous submission was deferred when the net-next tree closed during review.
- Corresponding iproute2 patch updated in response to review comments; no changes required for this patch.
v2:
- No changes since v1. (https://lore.kernel.org/netdev/20260614125000.6058-3-hemendranaik@gmail.com/)
v1:
- https://lore.kernel.org/netdev/20260531125314.22492-3-hemendranaik@gmail.com/
---
.../tc-testing/tc-tests/qdiscs/fq_pie.json | 22 +++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_pie.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_pie.json
index 229fe1bf4a90..88139f429430 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_pie.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq_pie.json
@@ -40,5 +40,27 @@
"matchPattern": "qdisc fq_pie 1: root refcnt [0-9]+ limit 1p",
"matchCount": "1",
"teardown": ["$TC qdisc del dev $DEV1 handle 1: root"]
+ },
+ {
+ "id": "83c0",
+ "name": "FQ-PIE class stats accessible via tc class show",
+ "category": [
+ "qdisc",
+ "fq_pie"
+ ],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "setup": [
+ "$TC qdisc add dev $DUMMY handle 1: root fq_pie"
+ ],
+ "cmdUnderTest": "$TC -s class show dev $DUMMY",
+ "expExitCode": "0",
+ "verifyCmd": "$TC -s class show dev $DUMMY",
+ "matchPattern": "class fq_pie",
+ "matchCount": "0",
+ "teardown": [
+ "$TC qdisc del dev $DUMMY handle 1: root"
+ ]
}
]
--
2.34.1
^ permalink raw reply related
* [PATCH iproute2-next v3] Add support for printing per-flow PIE statistics exposed by the kernel via the new TCA_FQ_PIE_XSTATS_CLASS type in tc_fq_pie_xstats.
From: Hemendra M. Naik @ 2026-06-30 18:44 UTC (permalink / raw)
To: netdev; +Cc: jiri, jhs, linux-kernel, vishy0777, tahiliani, Hemendra M. Naik
'tc -s class show' against an fq_pie qdisc now prints:
prob drop probability for the flow
delay per-flow queue sojourn time (microseconds)
deficit remaining DRR byte credits (signed integer)
avg_dq_rate dequeue rate estimate in bytes/second
(dq_rate_estimator mode only)
avg_dq_rate is formatted using tc_print_rate(), which converts the
kernel's bytes/second value to a human-readable bits/second string
(e.g. '3906Kbit'), consistent with how other tc schedulers display
rate fields. Apply the same fix to tc/q_pie.c, where avg_dq_rate was
also printed as a raw integer without a unit.
Update the UAPI header to mirror tc_fq_pie_cl_stats from the kernel.
Fix the 'delay' field comment in struct tc_pie_xstats from "in ms" to
"in microseconds" to match the kernel's
PSCHED_TICKS2NS / NSEC_PER_USEC conversion.
Add a 'tc -s class show' example to tc-fq_pie(8) with dq_rate_estimator
enabled, showing all per-flow fields (prob, delay, deficit, avg_dq_rate)
across multiple flows. Update tc-pie(8) avg_dq_rate example from a raw
integer to a formatted bits/second string.
The corresponding kernel patch can be viewed here:
https://lore.kernel.org/netdev/20260630183702.170798-1-hemendranaik@gmail.com/
Signed-off-by: Hemendra M. Naik <hemendranaik@gmail.com>
Signed-off-by: Vishal Kamath <vishy0777@gmail.com>
Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>
---
Changelog:
v3:
- Moved print_nl() into each xstats type block to preserve output formatting.
v2:
- Addressed ABI backward compatibility issue. (https://lore.kernel.org/netdev/20260614130729.10076-1-hemendranaik@gmail.com/)
v1:
- https://lore.kernel.org/netdev/20260531131411.28213-1-hemendranaik@gmail.com/
---
include/uapi/linux/pkt_sched.h | 29 +++++++++++++++--
man/man8/tc-fq_pie.8 | 18 +++++++++++
man/man8/tc-pie.8 | 2 +-
tc/q_fq_pie.c | 59 +++++++++++++++++++++++-----------
tc/q_pie.c | 4 +--
5 files changed, 88 insertions(+), 24 deletions(-)
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index d1f67ef8..f56f24ad 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -920,9 +920,9 @@ enum {
struct tc_pie_xstats {
__u64 prob; /* current probability */
- __u32 delay; /* current delay in ms */
+ __u32 delay; /* current delay in microseconds */
__u32 avg_dq_rate; /* current average dq_rate in
- * bits/pie_time
+ * bytes/second
*/
__u32 dq_rate_estimating; /* is avg_dq_rate being calculated? */
__u32 packets_in; /* total number of packets enqueued */
@@ -953,6 +953,25 @@ enum {
};
#define TCA_FQ_PIE_MAX (__TCA_FQ_PIE_MAX - 1)
+enum {
+ TCA_FQ_PIE_XSTATS_QDISC,
+ TCA_FQ_PIE_XSTATS_CLASS,
+};
+
+struct tc_fq_pie_cl_stats {
+ __u64 prob; /* current probability */
+ __u32 delay; /* current delay in microseconds */
+ __s32 deficit; /* number of remaining byte credits */
+ __u32 avg_dq_rate; /* current average dq_rate in
+ * bytes/second
+ */
+ __u32 dq_rate_estimating; /* is avg_dq_rate being calculated? */
+};
+
+struct tc_fq_pie_xqd_stats {
+ /* placeholder for new qdisc-level stats */
+};
+
struct tc_fq_pie_xstats {
__u32 packets_in; /* total number of packets enqueued */
__u32 dropped; /* packets dropped due to fq_pie_action */
@@ -963,6 +982,12 @@ struct tc_fq_pie_xstats {
__u32 new_flows_len; /* count of flows in new list */
__u32 old_flows_len; /* count of flows in old list */
__u32 memory_usage; /* total memory across all queues */
+ __u32 type;
+ union {
+ struct tc_fq_pie_cl_stats class_stats;
+ struct tc_fq_pie_xqd_stats xqdisc_stats;
+ };
+
};
/* CBS */
diff --git a/man/man8/tc-fq_pie.8 b/man/man8/tc-fq_pie.8
index 457a56bb..bf988f5f 100644
--- a/man/man8/tc-fq_pie.8
+++ b/man/man8/tc-fq_pie.8
@@ -153,6 +153,24 @@ dq_rate_estimator
pkts_in 6082 overlimit 0 overmemory 0 dropped 4 ecn_mark 0
new_flow_count 94 new_flows_len 0 old_flows_len 8 memory_used 1157632
+# tc qdisc add dev eth0 parent 100:1 handle 200: fq_pie target 2ms flows 3
+.br
+# tc -s class show dev eth0
+.br
+class fq_pie 200:2 parent 200:
+ Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
+ backlog 22800b 2p requeues 0
+ prob 0.57679 delay 2.38ms
+
+# tc qdisc add dev eth0 parent 100:1 handle 200: fq_pie target 2ms flows 3 dq_rate_estimator
+.br
+# tc -s class show dev eth0
+.br
+class fq_pie 200:2 parent 200:
+ Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
+ backlog 22800b 2p requeues 0
+ prob 0.57679 delay 2.38ms avg_dq_rate 10742Kbit
+
.SH SEE ALSO
.BR tc (8),
.BR tc-pie (8),
diff --git a/man/man8/tc-pie.8 b/man/man8/tc-pie.8
index 5a8c7820..f3a09616 100644
--- a/man/man8/tc-pie.8
+++ b/man/man8/tc-pie.8
@@ -115,7 +115,7 @@ is turned off.
qdisc pie 8036: dev eth0 root refcnt 2 limit 1000p target 15.0ms tupdate 16.0ms alpha 2 beta 20
Sent 63947420 bytes 42414 pkt (dropped 41, overlimits 0 requeues 0)
backlog 271006b 179p requeues 0
- prob 0.000092 delay 22200us avg_dq_rate 12145996
+ prob 0.000092 delay 22200us avg_dq_rate 10742Kbit
pkts_in 41 overlimit 343 dropped 0 maxq 50 ecn_mark 0
# tc qdisc add dev eth0 root pie limit 100 target 20ms tupdate 30ms ecn
diff --git a/tc/q_fq_pie.c b/tc/q_fq_pie.c
index dc2710cd..1b89ee3a 100644
--- a/tc/q_fq_pie.c
+++ b/tc/q_fq_pie.c
@@ -274,6 +274,8 @@ static int fq_pie_print_xstats(const struct qdisc_util *qu, FILE *f,
{
struct tc_fq_pie_xstats _st = {}, *st;
+ SPRINT_BUF(b1);
+
if (xstats == NULL)
return 0;
@@ -283,25 +285,44 @@ static int fq_pie_print_xstats(const struct qdisc_util *qu, FILE *f,
st = &_st;
}
- print_uint(PRINT_ANY, "pkts_in", " pkts_in %u",
- st->packets_in);
- print_uint(PRINT_ANY, "overlimit", " overlimit %u",
- st->overlimit);
- print_uint(PRINT_ANY, "overmemory", " overmemory %u",
- st->overmemory);
- print_uint(PRINT_ANY, "dropped", " dropped %u",
- st->dropped);
- print_uint(PRINT_ANY, "ecn_mark", " ecn_mark %u",
- st->ecn_mark);
- print_nl();
- print_uint(PRINT_ANY, "new_flow_count", " new_flow_count %u",
- st->new_flow_count);
- print_uint(PRINT_ANY, "new_flows_len", " new_flows_len %u",
- st->new_flows_len);
- print_uint(PRINT_ANY, "old_flows_len", " old_flows_len %u",
- st->old_flows_len);
- print_uint(PRINT_ANY, "memory_used", " memory_used %u",
- st->memory_usage);
+ if (!st->type || st->type == TCA_FQ_PIE_XSTATS_QDISC) {
+ print_uint(PRINT_ANY, "pkts_in", " pkts_in %u",
+ st->packets_in);
+ print_uint(PRINT_ANY, "overlimit", " overlimit %u",
+ st->overlimit);
+ print_uint(PRINT_ANY, "overmemory", " overmemory %u",
+ st->overmemory);
+ print_uint(PRINT_ANY, "dropped", " dropped %u",
+ st->dropped);
+ print_uint(PRINT_ANY, "ecn_mark", " ecn_mark %u",
+ st->ecn_mark);
+ print_nl();
+ print_uint(PRINT_ANY, "new_flow_count", " new_flow_count %u",
+ st->new_flow_count);
+ print_uint(PRINT_ANY, "new_flows_len", " new_flows_len %u",
+ st->new_flows_len);
+ print_uint(PRINT_ANY, "old_flows_len", " old_flows_len %u",
+ st->old_flows_len);
+ print_uint(PRINT_ANY, "memory_used", " memory_used %u",
+ st->memory_usage);
+ print_nl();
+ }
+
+ if (st->type == TCA_FQ_PIE_XSTATS_CLASS) {
+ print_float(PRINT_ANY, "prob", " prob %lg",
+ (double)st->class_stats.prob / (double)UINT64_MAX);
+ print_uint(PRINT_JSON, "delay", NULL, st->class_stats.delay);
+ print_string(PRINT_FP, NULL, " delay %s",
+ sprint_time(st->class_stats.delay, b1));
+ print_int(PRINT_ANY, "deficit", " deficit %d",
+ st->class_stats.deficit);
+
+ if (st->class_stats.dq_rate_estimating) {
+ tc_print_rate(PRINT_ANY, "avg_dq_rate", " avg_dq_rate %s",
+ st->class_stats.avg_dq_rate);
+ }
+ print_nl();
+ }
return 0;
diff --git a/tc/q_pie.c b/tc/q_pie.c
index 04c9aa61..abae1ced 100644
--- a/tc/q_pie.c
+++ b/tc/q_pie.c
@@ -220,8 +220,8 @@ static int pie_print_xstats(const struct qdisc_util *qu, FILE *f,
print_string(PRINT_FP, NULL, " delay %s", sprint_time(st->delay, b1));
if (st->dq_rate_estimating)
- print_uint(PRINT_ANY, "avg_dq_rate", " avg_dq_rate %u",
- st->avg_dq_rate);
+ tc_print_rate(PRINT_ANY, "avg_dq_rate", " avg_dq_rate %s",
+ st->avg_dq_rate);
print_nl();
print_uint(PRINT_ANY, "pkts_in", " pkts_in %u", st->packets_in);
--
2.34.1
^ permalink raw reply related
* Re: [PATCH net-next 0/9] Switch support
From: Jakub Kicinski @ 2026-06-30 18:52 UTC (permalink / raw)
To: Ratheesh Kannoth
Cc: linux-kernel, netdev, andrew+netdev, davem, edumazet, pabeni,
sgoutham
In-Reply-To: <20260630024715.4124281-1-rkannoth@marvell.com>
On Tue, 30 Jun 2026 08:17:06 +0530 Ratheesh Kannoth wrote:
> Marvell OcteonTX2 switch hardware is capable of accelerating L2, L3, and
> flow. When representors are enabled through devlink, a logical port is
> created in switch hardware for each representor device.
There's a number of allocations here which are missing a NULL check.
I'm sure sashiko will also have semi-infinite number of complaints,
so please check that yourself.
--
pw-bot: cr
^ permalink raw reply
* Re: [PATCH net-next v7 5/5] veth: time-based BQL completion coalescing via ethtool tx-usecs
From: Simon Schippers @ 2026-06-30 19:07 UTC (permalink / raw)
To: Jonas Köppeler, hawk, netdev
Cc: kernel-team, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Stanislav Fomichev, linux-kernel, bpf
In-Reply-To: <c4154d9c-8930-436d-b8b8-01951e8bd9f4@tu-berlin.de>
On 6/30/26 16:00, Jonas Köppeler wrote:
> On 6/13/26 4:14 PM, Simon Schippers wrote:
>> On 6/12/26 10:35, hawk@kernel.org wrote:
>>> From: Simon Schippers <simon.schippers@tu-dortmund.de>
>>>
>>> Per-packet BQL completion forces DQL to converge on limit=2, causing
>>> excessive NAPI scheduling overhead and qdisc requeues.
>>>
>>> Accumulate BQL completions and flush them when a configurable time
>>> threshold (tx-usecs) is exceeded, letting DQL discover a limit that
>>> bounds actual queuing delay to the configured interval. Coalescing
>>> state persists across NAPI polls in struct veth_rq so completions can
>>> accumulate beyond a single budget=64 cycle.
>>>
>>> The flush condition is:
>>>
>>> state->time + bql_flush_ns <= current_time || state->n_bql > dql.limit
>>>
>>> Flushing when n_bql exceeds dql.limit handles BQL starvation.
>>>
>>> The comparison is strictly greater-than because netdev_tx_sent_queue()
>>> always lets the producer exceed the limit by one before it stops, so
>>> n_bql == dql.limit is a normal in-flight state. dql.limit lives in
>>> the same cacheline as the completion path, so the check is cheap.
>>>
>>> Add ethtool tx-usecs support for runtime tuning. Default is 100 us;
>>> setting tx-usecs to 0 disables coalescing and falls back to per-packet
>>> completion.
>>>
>>> ethtool -C <veth-dev> tx-usecs 500 # 500us coalescing
>>> ethtool -C <veth-dev> tx-usecs 0 # per-packet (no coalescing)
>>>
>>> Co-developed-by: Jesper Dangaard Brouer <hawk@kernel.org>
>>> Signed-off-by: Jesper Dangaard Brouer <hawk@kernel.org>
>>> Co-developed-by: Jonas Köppeler <j.koeppeler@tu-berlin.de>
>>> Signed-off-by: Jonas Köppeler <j.koeppeler@tu-berlin.de>
>>> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
>>> ---
>>> drivers/net/veth.c | 123 ++++++++++++++++++++++++++++++++++++++++++---
>>> 1 file changed, 117 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>>> index 2473f730734b..c62d87a8402c 100644
>>> --- a/drivers/net/veth.c
>>> +++ b/drivers/net/veth.c
>>> @@ -28,6 +28,7 @@
>>> #include <linux/bpf_trace.h>
>>> #include <linux/net_tstamp.h>
>>> #include <linux/skbuff_ref.h>
>>> +#include <linux/sched/clock.h>
>>> #include <net/page_pool/helpers.h>
>>> #define DRV_NAME "veth"
>>> @@ -50,6 +51,7 @@
>>> * delay => 64 * 250 ms = 16 s.
>>> */
>>> #define VETH_WATCHDOG_TIMEOUT_MS (64 * 250)
>>> +#define VETH_BQL_COAL_TX_USECS 100 /* default tx-usecs for BQL batching*/
>>> struct veth_stats {
>>> u64 rx_drops;
>>> @@ -69,6 +71,11 @@ struct veth_rq_stats {
>>> struct u64_stats_sync syncp;
>>> };
>>> +struct veth_bql_state {
>>> + u64 time; /* sched_clock() when current coalescing window started */
>>> + uint n_bql; /* BQL completions batched in the current window */
>>> +};
>>> +
>>> struct veth_rq {
>>> struct napi_struct xdp_napi;
>>> struct napi_struct __rcu *napi; /* points to xdp_napi when the latteris initialized */
>>> @@ -76,6 +83,7 @@ struct veth_rq {
>>> struct bpf_prog __rcu *xdp_prog;
>>> struct xdp_mem_info xdp_mem;
>>> struct veth_rq_stats stats;
>>> + struct veth_bql_state bql_state;
>>> bool rx_notify_masked;
>>> struct ptr_ring xdp_ring;
>>> struct xdp_rxq_info xdp_rxq;
>>> @@ -88,6 +96,7 @@ struct veth_priv {
>>> struct bpf_prog *_xdp_prog;
>>> struct veth_rq *rq;
>>> unsigned int requested_headroom;
>>> + unsigned int tx_coal_usecs; /* BQL completion coalescing */
>>> };
>>> struct veth_xdp_tx_bq {
>>> @@ -272,7 +281,56 @@ static void veth_get_channels(struct net_device *dev,
>>> static int veth_set_channels(struct net_device *dev,
>>> struct ethtool_channels *ch);
>>> +static int veth_get_coalesce(struct net_device *dev,
>>> + struct ethtool_coalesce *ec,
>>> + struct kernel_ethtool_coalesce *kernel_coal,
>>> + struct netlink_ext_ack *extack)
>>> +{
>>> + struct veth_priv *priv = netdev_priv(dev);
>>> +
>>> + ec->tx_coalesce_usecs = priv->tx_coal_usecs;
>>> + return 0;
>>> +}
>>> +
>>> +static int veth_set_coalesce(struct net_device *dev,
>>> + struct ethtool_coalesce *ec,
>>> + struct kernel_ethtool_coalesce *kernel_coal,
>>> + struct netlink_ext_ack *extack)
>>> +{
>>> + struct veth_priv *priv = netdev_priv(dev);
>>> + struct net_device *peer;
>>> +
>>> + /* The coalescing window delays BQL completions, so keep tx-usecs well
>>> + * below the tx_timeout watchdog; otherwise a large value could stall a
>>> + * stopped queue long enough to trip a false watchdog timeout. Cap at
>>> + * half the watchdog to leave a generous safety margin. tx-usecs is
>>> + * microseconds, the watchdog is milliseconds.
>>> + */
>>> + if (ec->tx_coalesce_usecs > VETH_WATCHDOG_TIMEOUT_MS / 2 * USEC_PER_MSEC) {
>>> + NL_SET_ERR_MSG_MOD(extack,
>>> + "tx-usecs must stay below half the tx_timeout watchdog");
>>> + return -ERANGE;
>>> + }
>>> +
>>> + /* Paired with READ_ONCE in veth_xdp_rcv(). */
>>> + WRITE_ONCE(priv->tx_coal_usecs, ec->tx_coalesce_usecs);
>>> +
>>> + /* veth_xdp_rcv() reads each device's own value, so mirror it onto
>>> + * the peer to keep the pair symmetric: both directions coalesce
>>> + * with the same tx-usecs. Called under RTNL, rtnl_dereference() is safe.
>>> + */
>>> + peer = rtnl_dereference(priv->peer);
>>> + if (peer) {
>>> + struct veth_priv *peer_priv = netdev_priv(peer);
>>> +
>>> + WRITE_ONCE(peer_priv->tx_coal_usecs, ec->tx_coalesce_usecs);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static const struct ethtool_ops veth_ethtool_ops = {
>>> + .supported_coalesce_params = ETHTOOL_COALESCE_TX_USECS,
>>> .get_drvinfo = veth_get_drvinfo,
>>> .get_link = ethtool_op_get_link,
>>> .get_strings = veth_get_strings,
>>> @@ -282,6 +340,8 @@ static const struct ethtool_ops veth_ethtool_ops ={
>>> .get_ts_info = ethtool_op_get_ts_info,
>>> .get_channels = veth_get_channels,
>>> .set_channels = veth_set_channels,
>>> + .get_coalesce = veth_get_coalesce,
>>> + .set_coalesce = veth_set_coalesce,
>>> };
>>> /* general routines */
>>> @@ -969,13 +1029,54 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
>>> return NULL;
>>> }
>>> +static void veth_bql_maybe_complete(struct veth_bql_state *state,
>>> + struct netdev_queue *peer_txq,
>>> + u64 bql_flush_ns)
>>> +{
>>> + u64 current_time;
>>> +
>>> + /* There is no reason to complete with 0 and
>>> + * peer_txq could go away.
>>> + */
>>> + if (!state->n_bql || !peer_txq)
>>> + return;
>>> +
>>> + current_time = sched_clock();
>>> +
>>> + /* We complete if:
>>> + * 1. We reach bql_flush_ns.
>>> + * 2. We potentially have BQL starvation.
>>> + */
>>> + if (state->time + bql_flush_ns <= current_time ||
>>> + state->n_bql > peer_txq->dql.limit) {
>>
> Indeed, this does not compile when CONFIG_BQL is not set. I think we should just bring back the 'queue is empty + queue is stopped' check from v6 back at the end of the poll and remove the n_bql > dql.limit check.
We would put #ifdef CONFIG_BQL around that logic aswell.
> It also feels not obvious why this is handling the starvation case. This only works, because the producer has went overlimit previously and was stopped. So more than 'limit' packets have been enqueued to the ring, and they are eventually drained when this check is true.
I think it just needs some comment tweaking:
/* We complete if:
* 1. We reach bql_flush_ns.
* 2. We have BQL starvation. This means that the queue was over-limit
* in the last interval, and there is no more data in the queue,
* which is equivalent to we consumed more than limit items.
*/
> By removing this we can also avoid accessing dql internal members, but if you don't think that's a problem we can leave as is.
I agree accessing dql internal variables is not perfect.
That is why I have locally implemented DQL for software interfaces in
a generic way inside dynamic_queue_limits.{h,c}.
I was able to squeeze the time and n_bql variables into the completion
cacheline of the dql struct by moving around variables.
The logic applies inside dql_completed() if enabled.
With this we just have to call netdev_completed_queue().
Also it allows for per-queue tweaking of tx_usecs via sysfs.
Works well for me, can share it if we want to use it.
>
> Further, this is only works if VETH_BQL_UNIT stays 1, otherwise it will never fire. Anyway, still its necessary to check for CONFIG_BQL. But we could solve this by adding VETH_BQL_UNIT to n_bql instead of 1. This is also safe from any overflows, since limit is bound to limit_max, inflight is always less than limit + 1*VETH_BQL_UNIT and n_bql <= inflight.
You are right.
But I think there is no reason for VETH_BQL_UNIT anyway.
There should be no difference in the BQL algorithm, I personally
would replace VETH_BQL_UNIT with a hard-coded 1.
>
> In a version of bringing back the 'queue-empty' check and keeping most of the current logic (so a mixture of v6 and v7) resulted in the same performance on an x86_64 architecture.
>
>> Both Sashiko-Nipa and Sashiko-Gemini are right, this is missing a
>> #ifdef CONFIG_BQL. Not sure what is the best way to add them.
>> And for the struct we could maybe do:
>>
>> #ifdef CONFIG_BQL
>> struct veth_bql_state {
>> u64 time; /* sched_clock() when current coalescing window started */
>> uint n_bql; /* BQL completions batched in the current window */
>> };
>> #else
>> struct veth_bql_state {};
>> #endif
> Regarding the configs: we can just do something along those lines.
> struct veth_rq {
> ...
> #ifdef CONFIG_BQL
> struct veth_bql_state dql;
> #endif
> ...
> }
>
> and we put the rest of the code that accesses or performs an action regarding bql in some functions and do it like in netdev_* functions with
>
> Function-Signature()
> {
> #ifdef CONFIG_BQL
> // Code
> #endif
> }
>
> Wdyt?
> - Jonas
Yes, we have to. Unless we put it into dynamic_queue_limits.{h,c}
of course :^)
Thanks,
Simon
>>
>>> + netdev_tx_completed_queue(peer_txq, state->n_bql,
>>> + state->n_bql * VETH_BQL_UNIT);
>>> + state->time = current_time;
>>> + state->n_bql = 0;
>>> + }
>>> +}
>>> +
>>> static int veth_xdp_rcv(struct veth_rq *rq, int budget,
>>> struct veth_xdp_tx_bq *bq,
>>> struct veth_stats *stats,
>>> struct netdev_queue *peer_txq)
>>> {
>>> + struct veth_priv *priv = netdev_priv(rq->dev);
>>> + struct veth_bql_state *state = &rq->bql_state;
>>> int i, done = 0, n_xdpf = 0;
>>> void *xdpf[VETH_XDP_BATCH];
>>> + u64 bql_flush_ns;
>>> +
>>> + /* Mirrored to both peers; paired with WRITE_ONCE() in veth_set_coalesce */
>>> + bql_flush_ns = (u64)READ_ONCE(priv->tx_coal_usecs) * 1000;
>>> +
>>> + /* Clamp stored timestamp in case we migrated to a CPU with a behind
>>> + * sched_clock(); tries to reduce late BQL flushes.
>>> + */
>>> + state->time = min(state->time, sched_clock());
>>> +
>>> + /* Flush completions that timed out since the previous NAPI poll. */
>>> + veth_bql_maybe_complete(state, peer_txq, bql_flush_ns);>>
>>> for (i = 0; i < budget; i++) {
>>> void *ptr = __ptr_ring_consume(&rq->xdp_ring);
>>> @@ -1000,12 +1101,11 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
>>> }
>>> } else {
>>> /* ndo_start_xmit */
>>> - bool bql_charged = veth_ptr_is_bql(ptr);
>>> struct sk_buff *skb = veth_ptr_to_skb(ptr);
>>> + if (veth_ptr_is_bql(ptr))
>>> + state->n_bql++;
>>> stats->xdp_bytes += skb->len;
>>> - if (peer_txq && bql_charged)
>>> - netdev_tx_completed_queue(peer_txq, 1, VETH_BQL_UNIT);
>>> skb = veth_xdp_rcv_skb(rq, skb, bq, stats);
>>> if (skb) {
>>> @@ -1015,6 +1115,7 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
>>> napi_gro_receive(&rq->xdp_napi, skb);
>>> }
>>> }
>>> + veth_bql_maybe_complete(state, peer_txq, bql_flush_ns);
>>> done++;
>>
>> Sashiko-Nipa reports:
>>
>> "If veth_xdp_rcv() finishes and returns a done count less than the budget,
>> NAPI will go to sleep in veth_poll(). Do we need to unconditionally flush
>> any stranded BQL completions in veth_poll() before sleeping?
>> If completions are left in rq->bql_state indefinitely across NAPI idle
>> periods, it might present an artificially massive delay to DQL. This could
>> cause DQL to mistakenly conclude the hardware is extremely slow and
>> aggressively shrink dql.limit to its minimum, crippling throughput on
>> subsequent bursts."
>>
>> Again the issue that I found to be non-problematic in [1] and can be
>> seen by an BQL inflight > 0 when for example pktgen suddenly stops.
>>
>> If we would "unconditionally flush any stranded BQL completions in
>> veth_poll() before sleeping" we would *not* accumulate BQL completions
>> across NAPI polls but we want to do that.
>>
>> Do you agree?
>>
>> [1] https://lore.kernel.org/netdev/c8650d3a-e488-4279-b28f-549d766c23a1@tu-dortmund.de/
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox