On Thu, Aug 25, 2022 at 01:55:06PM +0200, Pankaj Raghav wrote:
> On Thu, Aug 25, 2022 at 01:48:43PM +0200, Pankaj Raghav wrote:
> > On Wed, Aug 24, 2022 at 01:14:40PM -0700, Keith Busch wrote:
> > >
> > > -static bool __sbq_wake_up(struct sbitmap_queue *sbq)
> > > +static bool __sbq_wake_up(struct sbitmap_queue *sbq, int *nr)
> > > {
> > > 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);
> >
> > I think the above statement is not needed if we use atomic_try_cmpxchg
> > as the old value is updated in that function itself.
> > https://docs.kernel.org/staging/index.html?highlight=atomic_try_cmpxchg#atomic-types
>
> I mean after moving the cur = atomic_read(..) above the do statement:
>
> wake_batch = READ_ONCE(sbq->wake_batch);
> cur = atomic_read(&ws->wait_cnt);
> do {
> sub = min3(wake_batch, *nr, cur);
> wait_cnt = cur - sub;
> } while (!atomic_try_cmpxchg(&ws->wait_cnt, &cur, wait_cnt));
Yeah, that's a good change. I'll fix it up.
