Re: [PATCH V4 1/2] cgroup/rstat: Helper functions for locking expose trylock

[Waiman Long <longman@xxxxxxxxxx>]

On 6/27/24 18:22, Waiman Long wrote:
> On 6/27/24 17:18, Jesper Dangaard Brouer wrote:
> > Signed-off-by: Jesper Dangaard Brouer <hawk@xxxxxxxxxx>
> > ---
> >   kernel/cgroup/rstat.c |   40 ++++++++++++++++++++++++++++++----------
> >   1 file changed, 30 insertions(+), 10 deletions(-)
> >
> > diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
> > index fb8b49437573..2a42be3a9bb3 100644
> > --- a/kernel/cgroup/rstat.c
> > +++ b/kernel/cgroup/rstat.c
> > @@ -279,17 +279,30 @@ __bpf_hook_end();
> >    * value -1 is used when obtaining the main lock else this is the CPU
> >    * number processed last.
> >    */
> > -static inline void __cgroup_rstat_lock(struct cgroup *cgrp, int 
> > cpu_in_loop)
> > +static inline bool __cgroup_rstat_trylock(struct cgroup *cgrp, int 
> > cpu_in_loop)
> > +{
> > +    bool locked;
> > +
> > +    locked = spin_trylock_irq(&cgroup_rstat_lock);
> > +    if (!locked)
> > +        trace_cgroup_rstat_lock_contended(cgrp, cpu_in_loop, !locked);
> > +
> > +    return locked;
> > +}
> > +
> > +static inline void __cgroup_rstat_lock(struct cgroup *cgrp, int 
> > cpu_in_loop,
> > +                       bool check_contention)
> >       __acquires(&cgroup_rstat_lock)
> >   {
> > -    bool contended;
> > +    bool locked = false;
> >   -    contended = !spin_trylock_irq(&cgroup_rstat_lock);
> > -    if (contended) {
> > -        trace_cgroup_rstat_lock_contended(cgrp, cpu_in_loop, 
> > contended);
> > +    if (check_contention)
> > +        locked = __cgroup_rstat_trylock(cgrp, cpu_in_loop);
> > +
> > +    if (!locked)
> >           spin_lock_irq(&cgroup_rstat_lock);
> > -    }
> > -    trace_cgroup_rstat_locked(cgrp, cpu_in_loop, contended);
> > +
> > +    trace_cgroup_rstat_locked(cgrp, cpu_in_loop, !locked);
> >   }
> >     static inline void __cgroup_rstat_unlock(struct cgroup *cgrp, int 
> > cpu_in_loop)
> > @@ -328,7 +341,7 @@ static void cgroup_rstat_flush_locked(struct 
> > cgroup *cgrp)
> >               __cgroup_rstat_unlock(cgrp, cpu);
> >               if (!cond_resched())
> >                   cpu_relax();
> > -            __cgroup_rstat_lock(cgrp, cpu);
> > +            __cgroup_rstat_lock(cgrp, cpu, true);
> >           }
> >       }
> >   }
> > @@ -348,9 +361,16 @@ static void cgroup_rstat_flush_locked(struct 
> > cgroup *cgrp)
> >    */
> >   __bpf_kfunc void cgroup_rstat_flush(struct cgroup *cgrp)
> >   {
> > +    bool locked;
> > +
> >       might_sleep();
> >   -    __cgroup_rstat_lock(cgrp, -1);
> > +    locked = __cgroup_rstat_trylock(cgrp, -1);
> > +    if (!locked) {
> > +        /* Opportunity to ongoing flush detection */
> > +        __cgroup_rstat_lock(cgrp, -1, false);
> > +    }
> > +
> >       cgroup_rstat_flush_locked(cgrp);
> >       __cgroup_rstat_unlock(cgrp, -1);
> >   }
> > @@ -368,7 +388,7 @@ void cgroup_rstat_flush_hold(struct cgroup *cgrp)
> >       __acquires(&cgroup_rstat_lock)
> >   {
> >       might_sleep();
> > -    __cgroup_rstat_lock(cgrp, -1);
> > +    __cgroup_rstat_lock(cgrp, -1, true);
> >       cgroup_rstat_flush_locked(cgrp);
> >   }
> Will it be cleaner to add a "bool *flushed" output parameter to 
> __cgroup_rstat_lock() so that the caller can respond differently 
> whether the flushed flag is set or not? In that way, you don't need to 
> expose a separate trylock() API. Also your commit log is empty.

Looking at the use case in patch 2, I would suggest the following APIs.

- bool cgroup_rstat_lock(struct cgroup *cgrp)
- bool cgroup_rstat_lock_or_flushed(struct cgroup *cgrp)

Both will return a bool indicating a flush has already been done if 
true. The 2nd function will not take the lock if a flush is ongoing. 
Both will wait if a flush is ongoing.

Cheers,
Longman