On Mon, Aug 28, 2023 at 10:43 AM Waiman Long <longman@xxxxxxxxxx> wrote: > > > On 8/28/23 13:35, Waiman Long wrote: > > On 8/28/23 13:28, Yosry Ahmed wrote: > >> On Mon, Aug 28, 2023 at 10:27 AM Waiman Long <longman@xxxxxxxxxx> wrote: > >>> > >>> On 8/28/23 13:07, Yosry Ahmed wrote: > >>>>> Here I agree with you. Let's go with the approach which is easy to > >>>>> undo for now. Though I prefer the new explicit interface for > >>>>> flushing, > >>>>> that step would be very hard to undo. Let's reevaluate if the > >>>>> proposed > >>>>> approach shows negative impact on production traffic and I think > >>>>> Cloudflare folks can give us the results soon. > >>>> Do you prefer we also switch to using a mutex (with preemption > >>>> disabled) to avoid the scenario Michal described where flushers give > >>>> up the lock and sleep resulting in an unbounded wait time in the worst > >>>> case? > >>> Locking with mutex with preemption disabled is an oxymoron. Use > >>> spinlock > >>> if you want to have preemption disabled. The purpose of usiing mutex is > >>> to allow the lock owner to sleep, but you can't sleep with preemption > >>> disabled. You need to enable preemption first. You can disable > >>> preemption for a short time in a non-sleeping section of the lock > >>> critical section, but I would not recommend disabling preemption for > >>> the > >>> whole critical section. > >> I thought using a mutex with preemption disabled would at least allow > >> waiters to sleep rather than spin, is this not correct (or doesn't > >> matter) ? > > > > Because of optimistic spinning, a mutex lock waiter will only sleep if > > the lock holder sleep or when its time slice run out. So the waiters > > are likely to spin for quite a while before they go to sleep. I see. Thanks for the explanation. > > Perhaps you can add a mutex at the read side so that only 1 reader can > contend with the global rstat spinlock at any time if this is a concern. I guess we can keep it simple for now and add that later if needed. For unified flushers we already can only have one. If we see a problem from the stat reading side we can add a mutex there. Thanks!
