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

From: Saravana Kannan
Date: Fri May 14 2021 - 14:39:36 EST


On Fri, May 14, 2021 at 11:33 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Fri, May 14, 2021 at 6:05 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> >
> > On Fri, May 14, 2021 at 5:12 AM Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>

-Saravana