> On Sat, 29 Jul 2023, Chuck Lever wrote: > > On Fri, Jul 28, 2023 at 08:44:04PM +0200, Lorenzo Bianconi wrote: [...] > > > > Neil said: > > > > > I suggest you add add a counter to the rqstp which is incremented from > > > even to odd after parsing a request - including he v4 parsing needed to > > > have a sable ->opcnt - and then incremented from odd to even when the > > > request is complete. > > > Then this code samples the counter, skips the rqst if the counter is > > > even, and resamples the counter after collecting the data. If it has > > > changed, the drop the record. > > > > I don't see a check if the status counter is even. > > ...and there does need to be one. If the counter is even, then the > fields are meaningless and unstable. The RQ_BUSY check is, I think, > meant to check if the fields are meaningful, but they aren't meaningful > until some time after RQ_BUSY is clear. > > I would replace the "RQ_BUSY not set" test with "counter is even" ack, I will fix it. > > > > > Also, as above, I'm not sure atomic_read() is necessary here. Maybe > > just READ_ONCE() ? Neil, any thoughts? > > Agree - we don't need an atomic as there is a single writer. > I think > smp_store_release(rqstp->counter, rqstp->counter|1) > to increment it after parsing the request. This makes it abundantly > clear the value will be odd, and ensures that if another thread sees the > 'odd' value, then it can also see the results of the parse. > > To increment after processing the request, > smp_store_release(rqstp->counter, rqstp->counter + 1) > > Then > counter = smp_load_acquire(rqstp->counter); > if ((counter & 1) == 0) > to test if it is even before reading the state. This ensure that if it > sees "odd' it will see the results of the parse. > > and > if ((smp_load_acquire(counter) == counter) continue; > > before trusting that the data we read was consistent. > > Note that we "release" *after* something and "acquire" *before" > something. > I think it helps to always think about what the access is "before" or > "after" when reasoning about barriers. > checkpatch will want a comment before these acquire and release > operation. I recommend using the corresponding word "before" or "after" > in that comment. ack, I will add it. Regards, Lorenzo > > NeilBrown >
Attachment:
signature.asc
Description: PGP signature
