Re: [PATCH wpan-next 08/11] net: mac802154: Add a warning in the hot path

From: Alexander Aring <alex.aring@gmail.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Stefan Schmidt <stefan@datenfreihafen.org>,
	linux-wpan - ML <linux-wpan@vger.kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	"open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>,
	David Girault <david.girault@qorvo.com>,
	Romuald Despres <romuald.despres@qorvo.com>,
	Frederic Blain <frederic.blain@qorvo.com>,
	Nicolas Schodet <nico@ni.fr.eu.org>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH wpan-next 08/11] net: mac802154: Add a warning in the hot path
Date: Wed, 27 Apr 2022 14:01:25 -0400	[thread overview]
Message-ID: <CAB_54W7NWEYgmLfowvyXtKEsKhBaVrPzpkB1kasYpAst98mKNA@mail.gmail.com> (raw)
In-Reply-To: <20220427164659.106447-9-miquel.raynal@bootlin.com>

Hi,

On Wed, Apr 27, 2022 at 12:47 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> We should never start a transmission after the queue has been stopped.
>
> But because it might work we don't kill the function here but rather
> warn loudly the user that something is wrong.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  net/mac802154/ieee802154_i.h |  8 ++++++++
>  net/mac802154/tx.c           |  2 ++
>  net/mac802154/util.c         | 18 ++++++++++++++++++
>  3 files changed, 28 insertions(+)
>
> diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> index b55fdefb0b34..cb61a4abaf37 100644
> --- a/net/mac802154/ieee802154_i.h
> +++ b/net/mac802154/ieee802154_i.h
> @@ -178,6 +178,14 @@ bool ieee802154_queue_is_held(struct ieee802154_local *local);
>   */
>  void ieee802154_stop_queue(struct ieee802154_local *local);
>
> +/**
> + * ieee802154_queue_is_stopped - check whether ieee802154 queue was stopped
> + * @local: main mac object
> + *
> + * Goes through all the interfaces and indicates if they are all stopped or not.
> + */
> +bool ieee802154_queue_is_stopped(struct ieee802154_local *local);
> +
>  /* MIB callbacks */
>  void mac802154_dev_set_page_channel(struct net_device *dev, u8 page, u8 chan);
>
> diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
> index a8a83f0167bf..021dddfea542 100644
> --- a/net/mac802154/tx.c
> +++ b/net/mac802154/tx.c
> @@ -124,6 +124,8 @@ bool ieee802154_queue_is_held(struct ieee802154_local *local)
>  static netdev_tx_t
>  ieee802154_hot_tx(struct ieee802154_local *local, struct sk_buff *skb)
>  {
> +       WARN_ON_ONCE(ieee802154_queue_is_stopped(local));
> +
>         return ieee802154_tx(local, skb);
>  }
>
> diff --git a/net/mac802154/util.c b/net/mac802154/util.c
> index 847e0864b575..cfd17a7db532 100644
> --- a/net/mac802154/util.c
> +++ b/net/mac802154/util.c
> @@ -44,6 +44,24 @@ void ieee802154_stop_queue(struct ieee802154_local *local)
>         rcu_read_unlock();
>  }
>
> +bool ieee802154_queue_is_stopped(struct ieee802154_local *local)
> +{
> +       struct ieee802154_sub_if_data *sdata;
> +       bool stopped = true;
> +
> +       rcu_read_lock();
> +       list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> +               if (!sdata->dev)
> +                       continue;
> +
> +               if (!netif_queue_stopped(sdata->dev))
> +                       stopped = false;
> +       }
> +       rcu_read_unlock();
> +
> +       return stopped;
> +}

sorry this makes no sense, you using net core functionality to check
if a queue is stopped in a net core netif callback. Whereas the sense
here for checking if the queue is really stopped is when 802.15.4
thinks the queue is stopped vs net core netif callback running. It
means for MLME-ops there are points we want to make sure that net core
is not handling any xmit and we should check this point and not
introducing net core functionality checks. btw: if it's hit your if
branch the first time you can break?

I am not done with the review, this is just what I see now and we can
discuss that. Please be patient.

- Alex