Re: Performance regression in GPIOLIB with SRCU when using the user-space ABI in a *wrong* way

["Paul E. McKenney" <paulmck@xxxxxxxxxx>]

On Mon, May 06, 2024 at 06:34:27PM +0200, Bartosz Golaszewski wrote:
> On Mon, May 6, 2024 at 3:55 PM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> >
> > On Mon, May 06, 2024 at 02:32:57PM +0200, Bartosz Golaszewski wrote:
> >>
> > > The offending kernel code looks like this:
> > >
> > >     old = rcu_replace_pointer(desc->label, new, 1);
> > >     synchronize_srcu(&desc->srcu);
> > >     kfree_const(old);
> > >
> > > I was wondering if we even have to synchronize here? The corresponding
> > > read-only sections call srcu_dereference(desc->label, &desc->srcu).
> > > Would it be enough to implement kfree_const_rcu() and use it here
> > > without synchronizing? Would the read-only users correctly see that
> > > last dereferenced address still points to valid memory until they all
> > > release the lock and the memory would only then be freed? Is my
> > > understanding of kfree_rcu() correct?
> >
> > It looks like kfree_const() just does a kfree(), so why not use
> > call_srcu() to replace the above calls to synchronize_srcu() and
> > kfree_const()?
> >
> > Something like this:
> >
> >         if (!is_kernel_rodata((unsigned long)(old)))
> >                 call_srcu(&desc->srcu, &desc->rh, gpio_cb);
> >
> > This requires adding an rcu_head field named "rh" to the structure
> > referenced by "desc" and creating a gpio_cb() wrapper function:
> >
> > static void connection_release(struct rcu_head *rhp)
> > {
> >         struct beats_me *bmp = container_of(rhp, struct beats_me, rh);
> >
> >         kfree(bmp);
> > }
> >
> > I could not find the code, so I don't know what "beats_me" above
> > should be replaced with.
> >
> > Would that work?
> 
> Thanks, this looks like a potential solution but something's not clear
> for me. What I want to free here is "old". However, its address is a
> field in struct beats_me and it's replaced (using
> rcu_replace_pointer()) before scheduling the free. When
> connection_release() will eventually be called, I think the address
> under bmp->label will be the new one? How would I pass some arbitrary
> user data to this callback (if at all possible?).

You are quite right, that "&desc->rh" should be "&old->rh", and the
struct rcu_head would need to go into the structure referenced by "old".

Apologies for my confusion, but in my defense, I could not find this code.

							Thanx, Paul