* [PATCH] net: mac802154: Fix racy device stats updates by DEV_STATS_INC() and DEV_STATS_ADD() @ 2024-05-31 8:07 Yunshui Jiang 2024-06-03 9:33 ` Stefan Schmidt 0 siblings, 1 reply; 6+ messages in thread From: Yunshui Jiang @ 2024-05-31 8:07 UTC (permalink / raw) To: linux-kernel, netdev, linux-wpan Cc: alex.aring, stefan, miquel.raynal, davem, Yunshui Jiang mac802154 devices update their dev->stats fields locklessly. Therefore these counters should be updated atomically. Adopt SMP safe DEV_STATS_INC() and DEV_STATS_ADD() to achieve this. Signed-off-by: Yunshui Jiang <jiangyunshui@kylinos.cn> --- net/mac802154/tx.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c index 2a6f1ed763c9..6fbed5bb5c3e 100644 --- a/net/mac802154/tx.c +++ b/net/mac802154/tx.c @@ -34,8 +34,8 @@ void ieee802154_xmit_sync_worker(struct work_struct *work) if (res) goto err_tx; - dev->stats.tx_packets++; - dev->stats.tx_bytes += skb->len; + DEV_STATS_INC(dev, tx_packets); + DEV_STATS_ADD(dev, tx_bytes, skb->len); ieee802154_xmit_complete(&local->hw, skb, false); @@ -90,8 +90,8 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb) if (ret) goto err_wake_netif_queue; - dev->stats.tx_packets++; - dev->stats.tx_bytes += len; + DEV_STATS_INC(dev, tx_packets); + DEV_STATS_ADD(dev, tx_bytes, len); } else { local->tx_skb = skb; queue_work(local->workqueue, &local->sync_tx_work); -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: mac802154: Fix racy device stats updates by DEV_STATS_INC() and DEV_STATS_ADD() 2024-05-31 8:07 [PATCH] net: mac802154: Fix racy device stats updates by DEV_STATS_INC() and DEV_STATS_ADD() Yunshui Jiang @ 2024-06-03 9:33 ` Stefan Schmidt 2024-06-03 23:55 ` Jakub Kicinski 0 siblings, 1 reply; 6+ messages in thread From: Stefan Schmidt @ 2024-06-03 9:33 UTC (permalink / raw) To: Yunshui Jiang, linux-kernel, netdev, linux-wpan Cc: alex.aring, miquel.raynal, davem Hello. On 31.05.24 10:07, Yunshui Jiang wrote: > mac802154 devices update their dev->stats fields locklessly. Therefore > these counters should be updated atomically. Adopt SMP safe DEV_STATS_INC() > and DEV_STATS_ADD() to achieve this. > > Signed-off-by: Yunshui Jiang <jiangyunshui@kylinos.cn> > --- > net/mac802154/tx.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c > index 2a6f1ed763c9..6fbed5bb5c3e 100644 > --- a/net/mac802154/tx.c > +++ b/net/mac802154/tx.c > @@ -34,8 +34,8 @@ void ieee802154_xmit_sync_worker(struct work_struct *work) > if (res) > goto err_tx; > > - dev->stats.tx_packets++; > - dev->stats.tx_bytes += skb->len; > + DEV_STATS_INC(dev, tx_packets); > + DEV_STATS_ADD(dev, tx_bytes, skb->len); > > ieee802154_xmit_complete(&local->hw, skb, false); > > @@ -90,8 +90,8 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb) > if (ret) > goto err_wake_netif_queue; > > - dev->stats.tx_packets++; > - dev->stats.tx_bytes += len; > + DEV_STATS_INC(dev, tx_packets); > + DEV_STATS_ADD(dev, tx_bytes, len); > } else { > local->tx_skb = skb; > queue_work(local->workqueue, &local->sync_tx_work); This patch has been applied to the wpan tree and will be part of the next pull request to net. Thanks! regards Stefan Schmidt ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: mac802154: Fix racy device stats updates by DEV_STATS_INC() and DEV_STATS_ADD() 2024-06-03 9:33 ` Stefan Schmidt @ 2024-06-03 23:55 ` Jakub Kicinski 2024-06-04 13:52 ` Alexander Aring 0 siblings, 1 reply; 6+ messages in thread From: Jakub Kicinski @ 2024-06-03 23:55 UTC (permalink / raw) To: Stefan Schmidt Cc: Yunshui Jiang, linux-kernel, netdev, linux-wpan, alex.aring, miquel.raynal, davem On Mon, 3 Jun 2024 11:33:28 +0200 Stefan Schmidt wrote: > Hello. > > On 31.05.24 10:07, Yunshui Jiang wrote: > > mac802154 devices update their dev->stats fields locklessly. Therefore > > these counters should be updated atomically. Adopt SMP safe DEV_STATS_INC() > > and DEV_STATS_ADD() to achieve this. > > > > Signed-off-by: Yunshui Jiang <jiangyunshui@kylinos.cn> > > --- > > net/mac802154/tx.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c > > index 2a6f1ed763c9..6fbed5bb5c3e 100644 > > --- a/net/mac802154/tx.c > > +++ b/net/mac802154/tx.c > > @@ -34,8 +34,8 @@ void ieee802154_xmit_sync_worker(struct work_struct *work) > > if (res) > > goto err_tx; > > > > - dev->stats.tx_packets++; > > - dev->stats.tx_bytes += skb->len; > > + DEV_STATS_INC(dev, tx_packets); > > + DEV_STATS_ADD(dev, tx_bytes, skb->len); > > > > ieee802154_xmit_complete(&local->hw, skb, false); > > > > @@ -90,8 +90,8 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb) > > if (ret) > > goto err_wake_netif_queue; > > > > - dev->stats.tx_packets++; > > - dev->stats.tx_bytes += len; > > + DEV_STATS_INC(dev, tx_packets); > > + DEV_STATS_ADD(dev, tx_bytes, len); > > } else { > > local->tx_skb = skb; > > queue_work(local->workqueue, &local->sync_tx_work); > > This patch has been applied to the wpan tree and will be > part of the next pull request to net. Thanks! Hi! I haven't looked in detail, but FWIW $ git grep LLTX net/mac802154/ $ and similar patch from this author has been rejected: https://lore.kernel.org/all/CANn89iLPYoOjMxNjBVHY7GwPFBGuxwRoM9gZZ-fWUUYFYjM1Uw@mail.gmail.com/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: mac802154: Fix racy device stats updates by DEV_STATS_INC() and DEV_STATS_ADD() 2024-06-03 23:55 ` Jakub Kicinski @ 2024-06-04 13:52 ` Alexander Aring 2024-06-05 20:02 ` Stefan Schmidt 0 siblings, 1 reply; 6+ messages in thread From: Alexander Aring @ 2024-06-04 13:52 UTC (permalink / raw) To: Jakub Kicinski Cc: Stefan Schmidt, Yunshui Jiang, linux-kernel, netdev, linux-wpan, alex.aring, miquel.raynal, davem Hi, On Mon, Jun 3, 2024 at 7:56 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 3 Jun 2024 11:33:28 +0200 Stefan Schmidt wrote: > > Hello. > > > > On 31.05.24 10:07, Yunshui Jiang wrote: > > > mac802154 devices update their dev->stats fields locklessly. Therefore > > > these counters should be updated atomically. Adopt SMP safe DEV_STATS_INC() > > > and DEV_STATS_ADD() to achieve this. > > > > > > Signed-off-by: Yunshui Jiang <jiangyunshui@kylinos.cn> > > > --- > > > net/mac802154/tx.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c > > > index 2a6f1ed763c9..6fbed5bb5c3e 100644 > > > --- a/net/mac802154/tx.c > > > +++ b/net/mac802154/tx.c > > > @@ -34,8 +34,8 @@ void ieee802154_xmit_sync_worker(struct work_struct *work) > > > if (res) > > > goto err_tx; > > > > > > - dev->stats.tx_packets++; > > > - dev->stats.tx_bytes += skb->len; > > > + DEV_STATS_INC(dev, tx_packets); > > > + DEV_STATS_ADD(dev, tx_bytes, skb->len); > > > > > > ieee802154_xmit_complete(&local->hw, skb, false); > > > > > > @@ -90,8 +90,8 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb) > > > if (ret) > > > goto err_wake_netif_queue; > > > > > > - dev->stats.tx_packets++; > > > - dev->stats.tx_bytes += len; > > > + DEV_STATS_INC(dev, tx_packets); > > > + DEV_STATS_ADD(dev, tx_bytes, len); > > > } else { > > > local->tx_skb = skb; > > > queue_work(local->workqueue, &local->sync_tx_work); > > > > This patch has been applied to the wpan tree and will be > > part of the next pull request to net. Thanks! > > Hi! I haven't looked in detail, but FWIW > > $ git grep LLTX net/mac802154/ > $ > > and similar patch from this author has been rejected: > > https://lore.kernel.org/all/CANn89iLPYoOjMxNjBVHY7GwPFBGuxwRoM9gZZ-fWUUYFYjM1Uw@mail.gmail.com/ In the case of ieee802154_tx() yes the tx lock is held so it's like what the mentioned link says. The workqueue is an ordered workqueue and you either have the driver async xmit option (the preferred option) or the driver sync xmit callback on a per driver (implies per interface) basis. I also don't see why there is currently a problem with the current non-atomic way. - Alex ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: mac802154: Fix racy device stats updates by DEV_STATS_INC() and DEV_STATS_ADD() 2024-06-04 13:52 ` Alexander Aring @ 2024-06-05 20:02 ` Stefan Schmidt 2024-06-05 21:54 ` Alexander Aring 0 siblings, 1 reply; 6+ messages in thread From: Stefan Schmidt @ 2024-06-05 20:02 UTC (permalink / raw) To: Alexander Aring, Jakub Kicinski Cc: Yunshui Jiang, linux-kernel, netdev, linux-wpan, alex.aring, miquel.raynal, davem Hello Jakub, Alex, On 04.06.24 15:52, Alexander Aring wrote: > Hi, > > On Mon, Jun 3, 2024 at 7:56 PM Jakub Kicinski <kuba@kernel.org> wrote: >> >> On Mon, 3 Jun 2024 11:33:28 +0200 Stefan Schmidt wrote: >>> Hello. >>> >>> On 31.05.24 10:07, Yunshui Jiang wrote: >>>> mac802154 devices update their dev->stats fields locklessly. Therefore >>>> these counters should be updated atomically. Adopt SMP safe DEV_STATS_INC() >>>> and DEV_STATS_ADD() to achieve this. >>>> >>>> Signed-off-by: Yunshui Jiang <jiangyunshui@kylinos.cn> >>>> --- >>>> net/mac802154/tx.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c >>>> index 2a6f1ed763c9..6fbed5bb5c3e 100644 >>>> --- a/net/mac802154/tx.c >>>> +++ b/net/mac802154/tx.c >>>> @@ -34,8 +34,8 @@ void ieee802154_xmit_sync_worker(struct work_struct *work) >>>> if (res) >>>> goto err_tx; >>>> >>>> - dev->stats.tx_packets++; >>>> - dev->stats.tx_bytes += skb->len; >>>> + DEV_STATS_INC(dev, tx_packets); >>>> + DEV_STATS_ADD(dev, tx_bytes, skb->len); >>>> >>>> ieee802154_xmit_complete(&local->hw, skb, false); >>>> >>>> @@ -90,8 +90,8 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb) >>>> if (ret) >>>> goto err_wake_netif_queue; >>>> >>>> - dev->stats.tx_packets++; >>>> - dev->stats.tx_bytes += len; >>>> + DEV_STATS_INC(dev, tx_packets); >>>> + DEV_STATS_ADD(dev, tx_bytes, len); >>>> } else { >>>> local->tx_skb = skb; >>>> queue_work(local->workqueue, &local->sync_tx_work); >>> >>> This patch has been applied to the wpan tree and will be >>> part of the next pull request to net. Thanks! >> >> Hi! I haven't looked in detail, but FWIW >> >> $ git grep LLTX net/mac802154/ >> $ >> >> and similar patch from this author has been rejected: >> >> https://lore.kernel.org/all/CANn89iLPYoOjMxNjBVHY7GwPFBGuxwRoM9gZZ-fWUUYFYjM1Uw@mail.gmail.com/ > > In the case of ieee802154_tx() yes the tx lock is held so it's like > what the mentioned link says. The workqueue is an ordered workqueue > and you either have the driver async xmit option (the preferred > option) or the driver sync xmit callback on a per driver (implies per > interface) basis. When I reviewed and applied this I did indeed not realize the ordered workqueue making this unnecessary. > I also don't see why there is currently a problem with the current > non-atomic way. For me this looks more like a wrapper that could avoid future problems for no cost. I would not mind if the patch stays. But you are right, its not strictly needed. Want me to back it out again? regards Stefan Schmidt ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: mac802154: Fix racy device stats updates by DEV_STATS_INC() and DEV_STATS_ADD() 2024-06-05 20:02 ` Stefan Schmidt @ 2024-06-05 21:54 ` Alexander Aring 0 siblings, 0 replies; 6+ messages in thread From: Alexander Aring @ 2024-06-05 21:54 UTC (permalink / raw) To: Stefan Schmidt Cc: Jakub Kicinski, Yunshui Jiang, linux-kernel, netdev, linux-wpan, alex.aring, miquel.raynal, davem Hi, On Wed, Jun 5, 2024 at 4:02 PM Stefan Schmidt <stefan@datenfreihafen.org> wrote: > > Hello Jakub, Alex, > > On 04.06.24 15:52, Alexander Aring wrote: > > Hi, > > > > On Mon, Jun 3, 2024 at 7:56 PM Jakub Kicinski <kuba@kernel.org> wrote: > >> > >> On Mon, 3 Jun 2024 11:33:28 +0200 Stefan Schmidt wrote: > >>> Hello. > >>> > >>> On 31.05.24 10:07, Yunshui Jiang wrote: > >>>> mac802154 devices update their dev->stats fields locklessly. Therefore > >>>> these counters should be updated atomically. Adopt SMP safe DEV_STATS_INC() > >>>> and DEV_STATS_ADD() to achieve this. > >>>> > >>>> Signed-off-by: Yunshui Jiang <jiangyunshui@kylinos.cn> > >>>> --- > >>>> net/mac802154/tx.c | 8 ++++---- > >>>> 1 file changed, 4 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c > >>>> index 2a6f1ed763c9..6fbed5bb5c3e 100644 > >>>> --- a/net/mac802154/tx.c > >>>> +++ b/net/mac802154/tx.c > >>>> @@ -34,8 +34,8 @@ void ieee802154_xmit_sync_worker(struct work_struct *work) > >>>> if (res) > >>>> goto err_tx; > >>>> > >>>> - dev->stats.tx_packets++; > >>>> - dev->stats.tx_bytes += skb->len; > >>>> + DEV_STATS_INC(dev, tx_packets); > >>>> + DEV_STATS_ADD(dev, tx_bytes, skb->len); > >>>> > >>>> ieee802154_xmit_complete(&local->hw, skb, false); > >>>> > >>>> @@ -90,8 +90,8 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb) > >>>> if (ret) > >>>> goto err_wake_netif_queue; > >>>> > >>>> - dev->stats.tx_packets++; > >>>> - dev->stats.tx_bytes += len; > >>>> + DEV_STATS_INC(dev, tx_packets); > >>>> + DEV_STATS_ADD(dev, tx_bytes, len); > >>>> } else { > >>>> local->tx_skb = skb; > >>>> queue_work(local->workqueue, &local->sync_tx_work); > >>> > >>> This patch has been applied to the wpan tree and will be > >>> part of the next pull request to net. Thanks! > >> > >> Hi! I haven't looked in detail, but FWIW > >> > >> $ git grep LLTX net/mac802154/ > >> $ > >> > >> and similar patch from this author has been rejected: > >> > >> https://lore.kernel.org/all/CANn89iLPYoOjMxNjBVHY7GwPFBGuxwRoM9gZZ-fWUUYFYjM1Uw@mail.gmail.com/ > > > > In the case of ieee802154_tx() yes the tx lock is held so it's like > > what the mentioned link says. The workqueue is an ordered workqueue > > and you either have the driver async xmit option (the preferred > > option) or the driver sync xmit callback on a per driver (implies per > > interface) basis. > > When I reviewed and applied this I did indeed not realize the ordered > workqueue making this unnecessary. > > > I also don't see why there is currently a problem with the current > > non-atomic way. > > For me this looks more like a wrapper that could avoid future problems > for no cost. I would not mind if the patch stays. But you are right, its > not strictly needed. Want me to back it out again? it's fine for me. I think atomic ops, depending on $ARCH will mess up your pipelines in some kind of way but it is better than using locks of course (but locks are here required for other things). - Alex ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-06-05 21:55 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-05-31 8:07 [PATCH] net: mac802154: Fix racy device stats updates by DEV_STATS_INC() and DEV_STATS_ADD() Yunshui Jiang 2024-06-03 9:33 ` Stefan Schmidt 2024-06-03 23:55 ` Jakub Kicinski 2024-06-04 13:52 ` Alexander Aring 2024-06-05 20:02 ` Stefan Schmidt 2024-06-05 21:54 ` Alexander Aring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).