Message-ID: <20230203161943.076ec169@xps-13>
Date:   Fri, 3 Feb 2023 16:19:43 +0100
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Alexander Aring <aahringo@...hat.com>
Cc:     Alexander Aring <alex.aring@...il.com>,
        Stefan Schmidt <stefan@...enfreihafen.org>,
        linux-wpan@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
        David Girault <david.girault@...vo.com>,
        Romuald Despres <romuald.despres@...vo.com>,
        Frederic Blain <frederic.blain@...vo.com>,
        Nicolas Schodet <nico@...fr.eu.org>,
        Guilhem Imberton <guilhem.imberton@...vo.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH wpan-next v2 0/2] ieee802154: Beaconing support

Hi Alexander,

aahringo@...hat.com wrote on Wed, 1 Feb 2023 12:15:42 -0500:

> Hi,
> 
> On Tue, Jan 31, 2023 at 6:25 AM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> >
> > Hi Alexander,
> >  
> > > > > > > Changes in v2:
> > > > > > > * Clearly state in the commit log llsec is not supported yet.
> > > > > > > * Do not use mlme transmission helpers because we don't really need to
> > > > > > >   stop the queue when sending a beacon, as we don't expect any feedback
> > > > > > >   from the PHY nor from the peers. However, we don't want to go through
> > > > > > >   the whole net stack either, so we bypass it calling the subif helper
> > > > > > >   directly.
> > > > > > >  
> > > > >
> > > > > moment, we use the mlme helpers to stop tx  
> > > >
> > > > No, we no longer use the mlme helpers to stop tx when sending beacons
> > > > (but true MLME transmissions, we ack handling and return codes will be
> > > > used for other purposes).
> > > >  
> > >
> > > then we run into an issue overwriting the framebuffer while the normal
> > > transmit path is active?  
> >
> > Crap, yes you're right. That's not gonna work.
> >
> > The net core acquires HARD_TX_LOCK() to avoid these issues and we are
> > no bypassing the net core without taking care of the proper frame
> > transmissions either (which would have worked with mlme_tx_one()). So I
> > guess there are two options:
> >
> > * Either we deal with the extra penalty of stopping the queue and
> >   waiting for the beacon to be transmitted with an mlme_tx_one() call,
> >   as proposed initially.
> >
> > * Or we hardcode our own "net" transmit helper, something like:
> >
> > mac802154_fast_mlme_tx() {
> >         struct net_device *dev = skb->dev;
> >         struct netdev_queue *txq;
> >
> >         txq = netdev_core_pick_tx(dev, skb, NULL);
> >         cpu = smp_processor_id();
> >         HARD_TX_LOCK(dev, txq, cpu);
> >         if (!netif_xmit_frozen_or_drv_stopped(txq))
> >                 netdev_start_xmit(skb, dev, txq, 0);
> >         HARD_TX_UNLOCK(dev, txq);
> > }
> >
> > Note1: this is very close to generic_xdp_tx() which tries to achieve the
> > same goal: sending packets, bypassing qdisc et al. I don't know whether
> > it makes sense to define it under mac802154/tx.c or core/dev.c and give
> > it another name, like generic_tx() or whatever would be more
> > appropriate. Or even adapting generic_xdp_tx() to make it look more
> > generic and use that function instead (without the xdp struct pointer).
> >  
> 
> The problem here is that the transmit handling is completely
> asynchronous. Calling netdev_start_xmit() is not "transmit and wait
> until transmit is done", it is "start transmit here is the buffer" an
> interrupt is coming up to report transmit is done. Until the time the
> interrupt isn't arrived the framebuffer on the device is in use, we
> don't know when the transceiver is done reading it. Only after tx done
> isr. The time until the isr isn't arrived is for us a -EBUSY case due
> hardware resource limitation. Currently we do that with stop/wake
> queue to avoid calling of xmit_do() to not run into such -EBUSY
> cases...
> 
> There might be clever things to do here to avoid this issue... I am
> not sure how XDP does that.
> 
> > Note2: I am wondering if it makes sense to disable bh here as well?  
> 
> May HARD_TX_LOCK() already do that? If they use spin_lock_bh() they
> disable local softirqs until the lock isn't held anymore.

I saw a case where both are called so I guess the short answer is "no":
https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L4307

> 
> >
> > Once we settle, I send a patch.
> >  
> 
> Not sure how to preceded here, but do see the problem? Or maybe I
> overlooked something here...

No you clearly had a sharp eye on that one, I totally see the problem.

Maybe the safest and simplest approach would be to be back using
the proper mlme transmission helpers for beacons (like in the initial
proposal). TBH I don't think there is a huge performance hit because in
both cases we wait for that ISR saying "the packet has been consumed by
the transceiver". It's just that in one case we wait for the return
code (MLME) and then return, in the other case we return but no
more packets will go through until the queue is released by the ISR (as
you said, in order to avoid the -EBUSY case). So in practice I don't
expect any performance hit. It is true however that we might want to
optimize this a little bit if we ever add something like an async
callback saying "skb consumed by the transceiver, another can be
queued" and gain a few us. Maybe a comment could be useful here (I'll
add it to my fix if we agree).

Thanks,
Miquèl

Powered by blists - more mailing lists