Re: [PATCH v1 2/2] drivers: base: Reduce device link removal code duplication

[Saravana Kannan <saravanak@google.com>]

On Fri, May 14, 2021 at 11:33 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, May 14, 2021 at 6:05 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Fri, May 14, 2021 at 5:12 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Reduce device link removal code duplication between the cases when
> > > SRCU is enabled and when it is disabled by moving the only differing
> > > piece of it (which is the removal of the link from the consumer and
> > > supplier lists) into a separate wrapper function (defined differently
> > > for each of the cases in question).
> > >
> > > No intentional functional impact.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/base/core.c |   31 +++++++++++++------------------
> > >  1 file changed, 13 insertions(+), 18 deletions(-)
> > >
> > > Index: linux-pm/drivers/base/core.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/base/core.c
> > > +++ linux-pm/drivers/base/core.c
> > > @@ -198,6 +198,12 @@ static void device_link_synchronize_remo
> > >  {
> > >         synchronize_srcu(&device_links_srcu);
> > >  }
> > > +
> > > +static void device_link_remove_from_lists(struct device_link *link)
> > > +{
> > > +       list_del_rcu(&link->s_node);
> > > +       list_del_rcu(&link->c_node);
> > > +}
> > >  #else /* !CONFIG_SRCU */
> > >  static DECLARE_RWSEM(device_links_lock);
> > >
> > > @@ -232,6 +238,12 @@ int device_links_read_lock_held(void)
> > >  static inline void device_link_synchronize_removal(void)
> > >  {
> > >  }
> > > +
> > > +static void device_link_remove_from_lists(struct device_link *link)
> > > +{
> > > +       list_del(&link->s_node);
> > > +       list_del(&link->c_node);
> > > +}
> > >  #endif /* !CONFIG_SRCU */
> > >
> > >  static bool device_is_ancestor(struct device *dev, struct device *target)
> > > @@ -854,7 +866,6 @@ out:
> > >  }
> > >  EXPORT_SYMBOL_GPL(device_link_add);
> > >
> > > -#ifdef CONFIG_SRCU
> > >  static void __device_link_del(struct kref *kref)
> > >  {
> > >         struct device_link *link = container_of(kref, struct device_link, kref);
> > > @@ -864,25 +875,9 @@ static void __device_link_del(struct kre
> > >
> > >         pm_runtime_drop_link(link);
> > >
> > > -       list_del_rcu(&link->s_node);
> > > -       list_del_rcu(&link->c_node);
> > > +       device_link_remove_from_lists(link);
> >
> > Remind me again why we can't do the synchronize_srcu() here (I'm not
> > too familiar with the SRCU API semantics)? Is it because
> > synchronize_srcu() can take indefinitely long?
>
> Not indefinitely, but it may take time.

More than if we had used normal mutex around these I suppose.

>  And because it is not
> actually useful before we end up freeing the device link memory.  And
> I'd rather not do it under the device links write lock.
>
> > I just vaguely remember
> > it does some checks during CPUs going idle (which can be a long time
> > later) but I'm not sure if that's the earliest you can synchronize. If
> > it's not indefinitely long and we just need to wait for other SRCU
> > critical sections to exit, maybe we can just synchronize here and make
> > the code a lot simpler?
>
> Well, maybe not  "a lot".
>
> > This function is anyway called in a sleepable context.
>
> But I'm not sure how long this context expects to be sleeping and
> sleeping under a mutex potentially blocks others.

Ack.

Reviewed-by: Saravana Kannan <saravanak@google.com>

-Saravana