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 07:46:18PM +0200, Bartosz Golaszewski wrote:
> On Mon, May 6, 2024 at 7:01 PM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> >
> > 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.
> 
> Sorry, I should have pointed to it in the first place. It lives in
> drivers/gpio/gpiolib.c in desc_set_label(). So "old" here is actually
> a const char * so I guess it should be made into a full-blown struct
> for this to work?

I should have been able to find that.  Maybe too early on Monday.  :-/

Anyway, yes, and one approach would be to have a structure containing
an rcu_head structure at the beginning and then the const char array
following that.  Please note that the rcu_head structure is modified by
call_srcu(), and thus cannot be const.

> Also: do I need to somehow manage struct rcu_head? Initialize/release
> it with some dedicated API? I couldn't find anything.

No management is needed unless the enclosing structure is allocated
on the stack.  In that case, you need topass a pointer to the rcu_head
structure to init_rcu_head_on_stack() before passing it to call_rcu()
and to destroy_rcu_head_on_stack() before the rcu_head structure goes
out of scope.

But in the usual case where the rcu_head structure is dynamically
allocated, no management, initialization, or cleanup is needed.

							Thanx, Paul