Messages in this thread | | | From | Saravana Kannan <> | Date | Fri, 14 May 2021 11:38:55 -0700 | Subject | Re: [PATCH v1 2/2] drivers: base: Reduce device link removal code duplication |
| |
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
| |