On 2/3/22 12:00 PM, Pavel Begunkov wrote: > On 2/3/22 18:29, Jens Axboe wrote: >> On 2/3/22 11:26 AM, Usama Arif wrote: >>> Hmm, maybe i didn't understand you and Pavel correctly. Are you >>> suggesting to do the below diff over patch 3? I dont think that would be >>> correct, as it is possible that just after checking if ctx->io_ev_fd is >>> present unregister can be called by another thread and set ctx->io_ev_fd >>> to NULL that would cause a NULL pointer exception later? In the current >>> patch, the check of whether ev_fd exists happens as the first thing >>> after rcu_read_lock and the rcu_read_lock are extremely cheap i believe. >> >> They are cheap, but they are still noticeable at high requests/sec >> rates. So would be best to avoid them. >> >> And yes it's obviously racy, there's the potential to miss an eventfd >> notification if it races with registering an eventfd descriptor. But >> that's not really a concern, as if you register with inflight IO >> pending, then that always exists just depending on timing. The only >> thing I care about here is that it's always _safe_. Hence something ala >> what you did below is totally fine, as we're re-evaluating under rcu >> protection. > > Indeed, the patch doesn't have any formal guarantees for propagation > to already inflight requests, so this extra unsynchronised check > doesn't change anything. > > I'm still more сurious why we need RCU and extra complexity when > apparently there is no use case for that. If it's only about > initial initialisation, then as I described there is a much > simpler approach. Would be nice if we could get rid of the quiesce code in general, but I haven't done a check to see what'd be missing after this... -- Jens Axboe
