Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API

[Johannes Berg <johannes@sipsolutions.net>]

On Mon, 2018-09-10 at 13:02 +0200, Toke Høiland-Jørgensen wrote:
> 
> > Is there a case where it's allowed to call this while *not* empty? At
> > least for the drivers it seems not, so perhaps when called from the
> > driver it should WARN() here or so?
> 
> Yeah, mac80211 calls this on wake_txq, where it will often be scheduled
> already. And since that can happen while drivers schedule stuff, it
> could also be the case that a driver re-schedules a queue that is
> already scheduled.

Right, I kinda gathered that but was thinking more from a driver POV as
I was reviewing, makes sense.

> > I'm just a bit worried that drivers will get this a bit wrong, and
> > we'll never know, but things won't work properly in some weird way.
> 
> What, subtle bugs in the data path that causes hangs in hard-to-debug
> cases? Nah, that never happens ;)

Good :-P

Actually though, we can't put a warn on there, because the driver might
take a txq, then mac80211 puts it on the list due to a new packet, and
then the driver also returns it.


> > > +	txqi = list_first_entry(&local->active_txqs[ac],
> > > +				struct txq_info,
> > > +				schedule_order);
> > > +
> > > +	if (txqi->schedule_round == local->schedule_round[ac])
> > > +		txqi = NULL;
> > 
> > that assigment seems unnecessary? txqi defaults to NULL.
> 
> No, this is an 'undo' of the previous line. I.e., we get the first txqi,
> check if we've already seen it this round, and if we have, unset txqi so
> the function returns NULL (causing the driver to stop looping).

Err, yeah, obviously.

johannes