From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtpout-04.galae.net (smtpout-04.galae.net [185.171.202.116]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 560F444CAD7 for ; Thu, 30 Apr 2026 16:20:17 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.171.202.116 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777566020; cv=none; b=o38ImPYRk83ahYaVGTfJ5A9T6CeKkuvV0R+S4mYhyMfvxdIIb7LtCbCUiTWyurqs0vxsn3uXwXWN5ueDO7vaFTteyi/9Gz2B+v3RbaU0L8AqzUS/VB0hBfCpUPH7Vtw+5YD9Nhc64pNygaBdNpPjhS38r5DZr2nAyj4sWbAcV1g= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777566020; c=relaxed/simple; bh=E2SKPUsUqU+3vPOonTNGjXVFFdcFDlAnxiNBfRQyCts=; h=Mime-Version:Content-Type:Date:Message-Id:Cc:To:From:Subject: References:In-Reply-To; b=jjdXHOEOPJcR4Xahdp1oBMK2uZBVqMkFv+syT2uzYPe3nsgQQlxU4efJnr4+YP+zixIeYvpU9Lrl44yGDwlMqrz5o4LAhFZ1lXKOLgWJnsdujlHYmFV/ymHyvLNMCoSQlX+0yKrpeAadxwumQojx+qhCFg2NqMkR7dAjtUpuHXc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com; spf=pass smtp.mailfrom=bootlin.com; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b=R6dIxE6U; arc=none smtp.client-ip=185.171.202.116 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=bootlin.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b="R6dIxE6U" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-04.galae.net (Postfix) with ESMTPS id 3163AC5CD6B; Thu, 30 Apr 2026 16:21:00 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id 67062601DF; Thu, 30 Apr 2026 16:20:15 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id E2EFE11AC9346; Thu, 30 Apr 2026 18:20:01 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1777566014; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:references; bh=3Bi+4lUG6gSEZp6P+IQVg5hHVjrg/DNa8f+Q/OrWakE=; b=R6dIxE6UeJHb3IJWoP5AjOqIMP5hS8HjvAxpqCFwoGDsIUyEc3UC+CufNMrVteeX5COYLa n5vuxJaWZUZCjNdkbxFHDgDOzGzLEMH9CMwbZPmarpJjnum8IJqP7B4EZ1s7RaOW2L7HGU BBoeZsHOROojOw13UHbyM8qZgA5Q7wURC937eIcEnxJ0mKBUTpZMWeal6lM3kgVQxGvc2p xea0gpF21AgpU9FxRG4O6JLvcFilPkvAoDC9oiY2x9ozIEhMb5jwvM03WDPLT0siT/7Q0x 0GxtL5cNowkDpIO3bQIihdWpA4PbX5peGjlOpQHAkx1VP8gqs6lOEdhaCGU9mg== Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 30 Apr 2026 18:20:01 +0200 Message-Id: Cc: "Nicolas Ferre" , "Claudiu Beznea" , "Andrew Lunn" , "David S. Miller" , "Eric Dumazet" , "Paolo Abeni" , "Haavard Skinnemoen" , "Jeff Garzik" , "Paolo Valerio" , "Conor Dooley" , "Nicolai Buchwitz" , , , "Vladimir Kondratiev" , "Gregory CLEMENT" , =?utf-8?q?Beno=C3=AEt_Monin?= , "Tawfik Bayouk" , "Thomas Petazzoni" , "Maxime Chevallier" , To: "Jakub Kicinski" From: =?utf-8?q?Th=C3=A9o_Lebrun?= Subject: Re: [PATCH net v2 2/4] net: macb: drop in-flight Tx SKBs on close X-Mailer: aerc 0.21.0-0-g5549850facc2 References: <20260428-macb-drop-tx-v2-0-647f5199d8df@bootlin.com> <20260428-macb-drop-tx-v2-2-647f5199d8df@bootlin.com> <20260429193446.5985abea@kernel.org> In-Reply-To: <20260429193446.5985abea@kernel.org> X-Last-TLS-Session-Version: TLSv1.3 Hello Jakub, On Thu Apr 30, 2026 at 4:34 AM CEST, Jakub Kicinski wrote: > On Tue, 28 Apr 2026 18:32:58 +0200 Th=C3=A9o Lebrun wrote: >> for (q =3D 0, queue =3D bp->queues; q < bp->num_queues; ++q, ++queue) = { >> - kfree(queue->tx_skb); >> - queue->tx_skb =3D NULL; >> + if (queue->tx_skb) { >> + unsigned int dropped =3D 0, tail; >> + >> + for (tail =3D queue->tx_tail; tail !=3D queue->tx_head; >> + tail++) { >> + if (macb_tx_skb(queue, tail)->skb) >> + dropped++; >> + macb_tx_unmap(bp, macb_tx_skb(queue, tail), 0, >> + SKB_DROP_REASON_NOT_SPECIFIED); >> + } >> + >> + queue->stats.tx_dropped +=3D dropped; >> + bp->dev->stats.tx_dropped +=3D dropped; > > I'm slightly baffled by the stats in this driver. > > Incrementing of both device and queue stats is highly unusual. > The driver seems to already have the values for the per-queue drops > but currently never increments it (did I miss it?) It does for Rx > stats but not for Tx stats. > > As sashiko correctly points out incrementing dev stats will lead > to races and lass of increments for multi-queue devices. > > Since there are no increments for tx_dropped stat today - could you > please delete it from ethtool -S, migrate the only existing > dev->stats.tx_dropped++; to increment the per-queue stat and make=20 > macb_get_stats() collect the tx_dropped from all queues, instead > of relying on the device-level stat? > > This should be patch 2 in this series, and then subsequent patches > don't have to do this double-counting dance. > > I suppose you may want to migrate the byte and packet counters > while at it, and add a u64 sync... Agreed. Here is the plan for next revision. It goes further than your proposal on some aspects and less so on others. - Stop using `netdev->stats`. Not even on MACB (single queue) or from at91 code (single queue, custom functions for a lot of things). This will drop all the double-counting; I added some in this series but there is lot in the driver to drop. sed 's/netdev->stats/queue->stats/' **/macb_main.c # -ish - All stats that used to land in netdev->stats will instead land in queue->stats. For that we need to add two fields: - multicast, incremented by at91ether_rx() - tx_errors, incremented by at91ether_interrupt() - Make queue->stats u64 values (getting inspiration from nstat). - In macb_get_stats(), replace: netdev_stats_to_stats64(nstat, &bp->dev->stats); by: for (q =3D 0, queue =3D bp->queues; q < bp->num_queues; ++q, ++queue)= { u64_stats_fetch_begin(...); nstat->rx_packets +=3D queue->stats.rx_packets; nstat->tx_packets +=3D queue->stats.tx_packets; // ... same for all stats ... } - Also the struct name (struct queue_stats) deserves a driver prefix. Notice we don't drop tx_dropped from `ethtool -S`. It might be useful to get per-queue stats and it doesn't cost much. We need per-queue counters anyway, let's keep exposing them. I don't have time to test enough, next revision will wait next week. Thanks, -- Th=C3=A9o Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com