On Thu, Aug 25, 2022 at 06:59:00AM -0600, Keith Busch wrote:
> On Thu, Aug 25, 2022 at 08:09:50AM +0200, Hannes Reinecke wrote:
> > > +static bool __sbq_wake_up(struct sbitmap_queue *sbq, int *nr)
> >
> > Why is '*nr' a pointer? It's always used as a value, so what does promoting
> > it to a point buys you?
>
> Not quite, see below:
>
> > > {
> > > struct sbq_wait_state *ws;
> > > - unsigned int wake_batch;
> > > - int wait_cnt;
> > > + int wake_batch, wait_cnt, sub, cur;
> > > ws = sbq_wake_ptr(sbq);
> > > if (!ws)
> > > return false;
> > > - wait_cnt = atomic_dec_return(&ws->wait_cnt);
> > > + wake_batch = READ_ONCE(sbq->wake_batch);
> > > + do {
> > > + cur = atomic_read(&ws->wait_cnt);
> > > + sub = min3(wake_batch, *nr, cur);
> > > + wait_cnt = cur - sub;
> > > + } while (!atomic_try_cmpxchg(&ws->wait_cnt, &cur, wait_cnt));
> > > +
> > > + *nr -= sub;
>
> Dereferenced here to account for how many bits were considered for this wake
> cycle.
Actually, I think we can just get rid of this accounting. Instead of calling
wake_up_nr() with wake_batch multiple times, we can call wake_up_nr() with
max(wake_batch, nr) just once, then all bits are accounted for in a single go.
The 'wake_batch' value is really just the lowest amount of tags to accumulate
before waking tag waiters; having more should be fine.
