| Message ID | 20220427164659.106447-7-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ieee802154: Synchronous Tx support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
Hi, On Wed, Apr 27, 2022 at 12:54 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Let's create a hold_txs atomic variable and increment/decrement it when > relevant. Currently we can use it during a suspend. Very soon we will > also use this feature during scans. > > When the variable is incremented, any further wake up call will be > skipped until the variable gets decremented back. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > include/net/cfg802154.h | 3 ++- > net/mac802154/cfg.c | 2 ++ > net/mac802154/ieee802154_i.h | 24 ++++++++++++++++++++++++ > net/mac802154/tx.c | 15 +++++++++++++++ > net/mac802154/util.c | 3 +++ > 5 files changed, 46 insertions(+), 1 deletion(-) > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h > index 473ebcb9b155..043d8e4359e7 100644 > --- a/include/net/cfg802154.h > +++ b/include/net/cfg802154.h > @@ -214,8 +214,9 @@ struct wpan_phy { > /* the network namespace this phy lives in currently */ > possible_net_t _net; > > - /* Transmission monitoring */ > + /* Transmission monitoring and control */ > atomic_t ongoing_txs; > + atomic_t hold_txs; > > char priv[] __aligned(NETDEV_ALIGN); > }; > diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c > index dafe02548161..0540a2b014d2 100644 > --- a/net/mac802154/cfg.c > +++ b/net/mac802154/cfg.c > @@ -46,6 +46,7 @@ static int ieee802154_suspend(struct wpan_phy *wpan_phy) > if (!local->open_count) > goto suspend; > > + ieee802154_hold_queue(local); > ieee802154_stop_queue(local); > synchronize_net(); > > @@ -72,6 +73,7 @@ static int ieee802154_resume(struct wpan_phy *wpan_phy) > return ret; > > wake_up: > + ieee802154_release_queue(local); > ieee802154_wake_queue(local); > local->suspended = false; > return 0; > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h > index 3f59a291b481..b55fdefb0b34 100644 > --- a/net/mac802154/ieee802154_i.h > +++ b/net/mac802154/ieee802154_i.h > @@ -142,6 +142,30 @@ enum hrtimer_restart ieee802154_xmit_ifs_timer(struct hrtimer *timer); > */ > void ieee802154_wake_queue(struct ieee802154_local *local); > > +/** > + * ieee802154_hold_queue - hold ieee802154 queue > + * @local: main mac object > + * > + * Hold a queue, this queue cannot be woken up while this is active. > + */ > +void ieee802154_hold_queue(struct ieee802154_local *local); > + > +/** > + * ieee802154_release_queue - release ieee802154 queue > + * @local: main mac object > + * > + * Release a queue which is held. The queue can now be woken up. > + */ > +void ieee802154_release_queue(struct ieee802154_local *local); > + > +/** > + * ieee802154_queue_is_held - checks whether the ieee802154 queue is held > + * @local: main mac object > + * > + * Checks whether the queue is currently held. > + */ > +bool ieee802154_queue_is_held(struct ieee802154_local *local); > + > /** > * ieee802154_stop_queue - stop ieee802154 queue > * @local: main mac object > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c > index 8c0bad7796ba..d088aa8119e8 100644 > --- a/net/mac802154/tx.c > +++ b/net/mac802154/tx.c > @@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb) > return NETDEV_TX_OK; > } > > +void ieee802154_hold_queue(struct ieee802154_local *local) > +{ > + atomic_inc(&local->phy->hold_txs); > +} > + > +void ieee802154_release_queue(struct ieee802154_local *local) > +{ > + atomic_dec(&local->phy->hold_txs); > +} > + > +bool ieee802154_queue_is_held(struct ieee802154_local *local) > +{ > + return atomic_read(&local->phy->hold_txs); > +} I am not getting this, should the release_queue() function not do something like: if (atomic_dec_and_test(hold_txs)) ieee802154_wake_queue(local); I think we don't need the test of "ieee802154_queue_is_held()" here, then we need to replace all stop_queue/wake_queue with hold and release? - Alex
Hi Alex, > > --- a/net/mac802154/tx.c > > +++ b/net/mac802154/tx.c > > @@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb) > > return NETDEV_TX_OK; > > } > > > > +void ieee802154_hold_queue(struct ieee802154_local *local) > > +{ > > + atomic_inc(&local->phy->hold_txs); > > +} > > + > > +void ieee802154_release_queue(struct ieee802154_local *local) > > +{ > > + atomic_dec(&local->phy->hold_txs); > > +} > > + > > +bool ieee802154_queue_is_held(struct ieee802154_local *local) > > +{ > > + return atomic_read(&local->phy->hold_txs); > > +} > > I am not getting this, should the release_queue() function not do > something like: > > if (atomic_dec_and_test(hold_txs)) > ieee802154_wake_queue(local); > > I think we don't need the test of "ieee802154_queue_is_held()" here, > then we need to replace all stop_queue/wake_queue with hold and > release? That's actually a good idea. I've implemented it and it looks nice too. I'll clean this up and share a new version with: - The wake call checked everytime hold_txs gets decremented - The removal of the _queue_is_held() helper - _wake/stop_queue() turned static - _hold/release_queue() used everywhere Thanks, Miquèl
Hi, On Tue, May 10, 2022 at 10:52 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alex, > > > > --- a/net/mac802154/tx.c > > > +++ b/net/mac802154/tx.c > > > @@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb) > > > return NETDEV_TX_OK; > > > } > > > > > > +void ieee802154_hold_queue(struct ieee802154_local *local) > > > +{ > > > + atomic_inc(&local->phy->hold_txs); > > > +} > > > + > > > +void ieee802154_release_queue(struct ieee802154_local *local) > > > +{ > > > + atomic_dec(&local->phy->hold_txs); > > > +} > > > + > > > +bool ieee802154_queue_is_held(struct ieee802154_local *local) > > > +{ > > > + return atomic_read(&local->phy->hold_txs); > > > +} > > > > I am not getting this, should the release_queue() function not do > > something like: > > > > if (atomic_dec_and_test(hold_txs)) > > ieee802154_wake_queue(local); > > > > I think we don't need the test of "ieee802154_queue_is_held()" here, > > then we need to replace all stop_queue/wake_queue with hold and > > release? > > That's actually a good idea. I've implemented it and it looks nice too. > I'll clean this up and share a new version with: > - The wake call checked everytime hold_txs gets decremented > - The removal of the _queue_is_held() helper > - _wake/stop_queue() turned static > - _hold/release_queue() used everywhere > I think there is also a lock necessary for atomic inc/dec hitting zero and the stop/wake call afterwards... ,there are also a lot of optimization techniques to only hold the lock for hitting zero cases in such areas. However we will see... - Alex
Hi Alexander, aahringo@redhat.com wrote on Wed, 11 May 2022 09:09:40 -0400: > Hi, > > On Tue, May 10, 2022 at 10:52 AM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > > > Hi Alex, > > > > > > --- a/net/mac802154/tx.c > > > > +++ b/net/mac802154/tx.c > > > > @@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb) > > > > return NETDEV_TX_OK; > > > > } > > > > > > > > +void ieee802154_hold_queue(struct ieee802154_local *local) > > > > +{ > > > > + atomic_inc(&local->phy->hold_txs); > > > > +} > > > > + > > > > +void ieee802154_release_queue(struct ieee802154_local *local) > > > > +{ > > > > + atomic_dec(&local->phy->hold_txs); > > > > +} > > > > + > > > > +bool ieee802154_queue_is_held(struct ieee802154_local *local) > > > > +{ > > > > + return atomic_read(&local->phy->hold_txs); > > > > +} > > > > > > I am not getting this, should the release_queue() function not do > > > something like: > > > > > > if (atomic_dec_and_test(hold_txs)) > > > ieee802154_wake_queue(local); > > > > > > I think we don't need the test of "ieee802154_queue_is_held()" here, > > > then we need to replace all stop_queue/wake_queue with hold and > > > release? > > > > That's actually a good idea. I've implemented it and it looks nice too. > > I'll clean this up and share a new version with: > > - The wake call checked everytime hold_txs gets decremented > > - The removal of the _queue_is_held() helper > > - _wake/stop_queue() turned static > > - _hold/release_queue() used everywhere > > > > I think there is also a lock necessary for atomic inc/dec hitting zero > and the stop/wake call afterwards... Mmmh that is true, it can race. I've introduced a mutex (I think it's safe but it can be turned into a spinlock if proven necessary) to secure these increment/decrement+wakeup operations. > ,there are also a lot of > optimization techniques to only hold the lock for hitting zero cases > in such areas. However we will see... I am not aware of technical solutions to avoid the locking in these cases, what do you have in mind? Otherwise I propose just to come up with a working and hopefully solid solution and then we'll see how to optimize. Thanks, Miquèl
Hi, On Thu, May 12, 2022 at 10:33 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Wed, 11 May 2022 09:09:40 -0400: > > > Hi, > > > > On Tue, May 10, 2022 at 10:52 AM Miquel Raynal > > <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Alex, > > > > > > > > --- a/net/mac802154/tx.c > > > > > +++ b/net/mac802154/tx.c > > > > > @@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb) > > > > > return NETDEV_TX_OK; > > > > > } > > > > > > > > > > +void ieee802154_hold_queue(struct ieee802154_local *local) > > > > > +{ > > > > > + atomic_inc(&local->phy->hold_txs); > > > > > +} > > > > > + > > > > > +void ieee802154_release_queue(struct ieee802154_local *local) > > > > > +{ > > > > > + atomic_dec(&local->phy->hold_txs); > > > > > +} > > > > > + > > > > > +bool ieee802154_queue_is_held(struct ieee802154_local *local) > > > > > +{ > > > > > + return atomic_read(&local->phy->hold_txs); > > > > > +} > > > > > > > > I am not getting this, should the release_queue() function not do > > > > something like: > > > > > > > > if (atomic_dec_and_test(hold_txs)) > > > > ieee802154_wake_queue(local); > > > > > > > > I think we don't need the test of "ieee802154_queue_is_held()" here, > > > > then we need to replace all stop_queue/wake_queue with hold and > > > > release? > > > > > > That's actually a good idea. I've implemented it and it looks nice too. > > > I'll clean this up and share a new version with: > > > - The wake call checked everytime hold_txs gets decremented > > > - The removal of the _queue_is_held() helper > > > - _wake/stop_queue() turned static > > > - _hold/release_queue() used everywhere > > > > > > > I think there is also a lock necessary for atomic inc/dec hitting zero > > and the stop/wake call afterwards... > > Mmmh that is true, it can race. I've introduced a mutex (I think it's > safe but it can be turned into a spinlock if proven necessary) to > secure these increment/decrement+wakeup operations. > be aware that you might call these functions from different contexts, test your patches with PROVE_LOCKING enabled. > > ,there are also a lot of > > optimization techniques to only hold the lock for hitting zero cases > > in such areas. However we will see... > > I am not aware of technical solutions to avoid the locking in these > cases, what do you have in mind? Otherwise I propose just to come up > with a working and hopefully solid solution and then we'll see how to > optimize. Yes, it's not so important... - Alex
Hi Alex, aahringo@redhat.com wrote on Thu, 12 May 2022 10:44:35 -0400: > Hi, > > On Thu, May 12, 2022 at 10:33 AM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > aahringo@redhat.com wrote on Wed, 11 May 2022 09:09:40 -0400: > > > > > Hi, > > > > > > On Tue, May 10, 2022 at 10:52 AM Miquel Raynal > > > <miquel.raynal@bootlin.com> wrote: > > > > > > > > Hi Alex, > > > > > > > > > > --- a/net/mac802154/tx.c > > > > > > +++ b/net/mac802154/tx.c > > > > > > @@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb) > > > > > > return NETDEV_TX_OK; > > > > > > } > > > > > > > > > > > > +void ieee802154_hold_queue(struct ieee802154_local *local) > > > > > > +{ > > > > > > + atomic_inc(&local->phy->hold_txs); > > > > > > +} > > > > > > + > > > > > > +void ieee802154_release_queue(struct ieee802154_local *local) > > > > > > +{ > > > > > > + atomic_dec(&local->phy->hold_txs); > > > > > > +} > > > > > > + > > > > > > +bool ieee802154_queue_is_held(struct ieee802154_local *local) > > > > > > +{ > > > > > > + return atomic_read(&local->phy->hold_txs); > > > > > > +} > > > > > > > > > > I am not getting this, should the release_queue() function not do > > > > > something like: > > > > > > > > > > if (atomic_dec_and_test(hold_txs)) > > > > > ieee802154_wake_queue(local); > > > > > > > > > > I think we don't need the test of "ieee802154_queue_is_held()" here, > > > > > then we need to replace all stop_queue/wake_queue with hold and > > > > > release? > > > > > > > > That's actually a good idea. I've implemented it and it looks nice too. > > > > I'll clean this up and share a new version with: > > > > - The wake call checked everytime hold_txs gets decremented > > > > - The removal of the _queue_is_held() helper > > > > - _wake/stop_queue() turned static > > > > - _hold/release_queue() used everywhere > > > > > > > > > > I think there is also a lock necessary for atomic inc/dec hitting zero > > > and the stop/wake call afterwards... > > > > Mmmh that is true, it can race. I've introduced a mutex (I think it's > > safe but it can be turned into a spinlock if proven necessary) to > > secure these increment/decrement+wakeup operations. > > > > be aware that you might call these functions from different contexts, > test your patches with PROVE_LOCKING enabled. Right, I've added it to my .config, let's see what it tells me. > > > ,there are also a lot of > > > optimization techniques to only hold the lock for hitting zero cases > > > in such areas. However we will see... > > > > I am not aware of technical solutions to avoid the locking in these > > cases, what do you have in mind? Otherwise I propose just to come up > > with a working and hopefully solid solution and then we'll see how to > > optimize. > > Yes, it's not so important... > > - Alex > Thanks, Miquèl
diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h index 473ebcb9b155..043d8e4359e7 100644 --- a/include/net/cfg802154.h +++ b/include/net/cfg802154.h @@ -214,8 +214,9 @@ struct wpan_phy { /* the network namespace this phy lives in currently */ possible_net_t _net; - /* Transmission monitoring */ + /* Transmission monitoring and control */ atomic_t ongoing_txs; + atomic_t hold_txs; char priv[] __aligned(NETDEV_ALIGN); }; diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c index dafe02548161..0540a2b014d2 100644 --- a/net/mac802154/cfg.c +++ b/net/mac802154/cfg.c @@ -46,6 +46,7 @@ static int ieee802154_suspend(struct wpan_phy *wpan_phy) if (!local->open_count) goto suspend; + ieee802154_hold_queue(local); ieee802154_stop_queue(local); synchronize_net(); @@ -72,6 +73,7 @@ static int ieee802154_resume(struct wpan_phy *wpan_phy) return ret; wake_up: + ieee802154_release_queue(local); ieee802154_wake_queue(local); local->suspended = false; return 0; diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h index 3f59a291b481..b55fdefb0b34 100644 --- a/net/mac802154/ieee802154_i.h +++ b/net/mac802154/ieee802154_i.h @@ -142,6 +142,30 @@ enum hrtimer_restart ieee802154_xmit_ifs_timer(struct hrtimer *timer); */ void ieee802154_wake_queue(struct ieee802154_local *local); +/** + * ieee802154_hold_queue - hold ieee802154 queue + * @local: main mac object + * + * Hold a queue, this queue cannot be woken up while this is active. + */ +void ieee802154_hold_queue(struct ieee802154_local *local); + +/** + * ieee802154_release_queue - release ieee802154 queue + * @local: main mac object + * + * Release a queue which is held. The queue can now be woken up. + */ +void ieee802154_release_queue(struct ieee802154_local *local); + +/** + * ieee802154_queue_is_held - checks whether the ieee802154 queue is held + * @local: main mac object + * + * Checks whether the queue is currently held. + */ +bool ieee802154_queue_is_held(struct ieee802154_local *local); + /** * ieee802154_stop_queue - stop ieee802154 queue * @local: main mac object diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c index 8c0bad7796ba..d088aa8119e8 100644 --- a/net/mac802154/tx.c +++ b/net/mac802154/tx.c @@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb) return NETDEV_TX_OK; } +void ieee802154_hold_queue(struct ieee802154_local *local) +{ + atomic_inc(&local->phy->hold_txs); +} + +void ieee802154_release_queue(struct ieee802154_local *local) +{ + atomic_dec(&local->phy->hold_txs); +} + +bool ieee802154_queue_is_held(struct ieee802154_local *local) +{ + return atomic_read(&local->phy->hold_txs); +} + netdev_tx_t ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev) { diff --git a/net/mac802154/util.c b/net/mac802154/util.c index 4bcc9cd2eb9d..847e0864b575 100644 --- a/net/mac802154/util.c +++ b/net/mac802154/util.c @@ -17,6 +17,9 @@ void ieee802154_wake_queue(struct ieee802154_local *local) { struct ieee802154_sub_if_data *sdata; + if (ieee802154_queue_is_held(local)) + return; + rcu_read_lock(); list_for_each_entry_rcu(sdata, &local->interfaces, list) { if (!sdata->dev)
Let's create a hold_txs atomic variable and increment/decrement it when relevant. Currently we can use it during a suspend. Very soon we will also use this feature during scans. When the variable is incremented, any further wake up call will be skipped until the variable gets decremented back. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- include/net/cfg802154.h | 3 ++- net/mac802154/cfg.c | 2 ++ net/mac802154/ieee802154_i.h | 24 ++++++++++++++++++++++++ net/mac802154/tx.c | 15 +++++++++++++++ net/mac802154/util.c | 3 +++ 5 files changed, 46 insertions(+), 1 deletion(-)