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.