On Thu, Oct 29, 2020 at 03:54:21PM -0500, Frank Rowand wrote: > On 10/28/20 11:25 AM, Michael Auchter wrote: > > Hey Saravana, > > > > Thanks for taking the time to look into this! > > > > On Mon, Oct 26, 2020 at 12:10:33PM -0700, Saravana Kannan wrote: > >> On Wed, Oct 21, 2020 at 2:02 PM Frank Rowand <frowand.list@xxxxxxxxx> wrote: > >>> > >>> Hi Saravana, > >>> > >>> Michael found an issue related to the removal of a devicetree node > >>> which involves devlinks: > >>> > >>> On 10/14/20 2:36 PM, Michael Auchter wrote: > >>>> After updating to v5.9, I've started seeing errors in the kernel log > >>>> when using device tree overlays. Specifically, the problem seems to > >>>> happen when removing a device tree overlay that contains two devices > >>>> with some dependency between them (e.g., a device that provides a clock > >>>> and a device that consumes that clock). Removing such an overlay results > >>>> in: > >>>> > >>>> OF: ERROR: memory leak, expected refcount 1 instead of 2, of_node_get()/of_node_put() unbalanced - destroy > >>>> OF: ERROR: memory leak, expected refcount 1 instead of 2, of_node_get()/of_node_put() unbalanced - destroy > >>>> > >>>> followed by hitting some REFCOUNT_WARNs in refcount.c > >>>> > >>>> In the first patch, I've included a unittest that can be used to > >>>> reproduce this when built with CONFIG_OF_UNITTEST [1]. > >>>> > >>>> I believe the issue is caused by the cleanup performed when releasing > >>>> the devlink device that's created to represent the dependency between > >>>> devices. The devlink device has references to the consumer and supplier > >>>> devices, which it drops in device_link_free; the devlink device's > >>>> release callback calls device_link_free via call_srcu. > >>>> > >>>> When the overlay is being removed, all devices are removed, and > >>>> eventually the release callback for the devlink device run, and > >>>> schedules cleanup using call_srcu. Before device_link_free can and call > >>>> put_device on the consumer/supplier, the rest of the overlay removal > >>>> process runs, resulting in the error traces above. > >>> > >>> When a devicetree node in an overlay is removed, the remove code expects > >>> all previous users of the related device to have done the appropriate put > >>> of the device and to have no later references. > >>> > >>> As Michael described above, the devlink release callback defers the > >>> put_device(). The cleanup via srcu was implemented in commit > >>> 843e600b8a2b01463c4d873a90b2c2ea8033f1f6 "driver core: Fix sleeping > >>> in invalid context during device link deletion" to solve yet another > >>> issue. > >>> > >>> > >>>> > >>>> Patches 2 and 3 are an attempt at fixing this: call srcu_barrier to wait > >>>> for any pending device_link_free's to execute before continuing on with > >>>> the removal process. > >>>> > >>>> These patches resolve the issue, but probably not in the best way. In > >>>> particular, it seems strange to need to leak details of devlinks into > >>>> the device tree overlay code. So, I'd be curious to get some feedback or > >>>> hear any other ideas for how to resolve this issue. > >>> > >>> I agree with Michael that adding an indirect call of srcu_barrier(&device_links_srcu) > >>> into the devicetree overlay code is not an appropriate solution. > >> > >> I kind of see your point too. I wonder if the srcu_barrier() should > >> happen inside like so: > >> device_del() -> device_links_purge()->srcu_barrier() > >> > >> I don't know what contention the use of SRCUs in device links was > >> trying to avoid, but I think the srcu_barrier() call path I suggested > >> above shouldn't be a problem. If that fixes the issue, the best way to > >> know if it's an issue is to send out a patch and see if Rafael has any > >> problem with it :) > > > > I was able to test this by adding the srcu_barrier() at the end of > > device_links_purge(), and that does seem to have fixed the issue. > > > >>> Is there some other way to fix the problem that 843e600b8a2b solves without > >>> deferring the put_device() done by the devlink release callback? > >> > >> Ok I finally got some time to look into this closely. > >> > >> Even if you revert 843e600b8a2b, you'll see that device_link_free() > >> (which drops the reference to the consumer and supplier devices) was > >> scheduled to run when the SRCU clean up occurs. So I think this issue > >> was present even before 843e600b8a2b, but commit 843e600b8a2b just > >> made it more likely to hit this scenario because it introduces some > >> delay in dropping the ref count of the supplier and consumer by going > >> through the device link device's release path. So, I think this issue > >> isn't related to 843e600b8a2b. > >> > >> As to why 843e600b8a2b had to be written to call call_srcu() from the > >> device link device's release path, it's a mess of dependencies/delays: > >> 1. The device link device is part of the struct device_link. So we > >> can't free device_link before the device_link.link_dev refcount goes > >> to 0. > >> 2. But I can't assume device_link.link_dev's refcount will go to 0 as > >> soon as I call put_device() on it because of > >> CONFIG_DEBUG_KOBJECT_RELEASE which frees up the kobject after a random > >> delay. > >> 3. The use of SRCU also means I can't free device_link until the SRCU > >> is cleaned up. > >> > >> Because of (1), (2) and (3), when the device_link_del() (or any of the > >> other device link deletion APIs are called) I first have to do a > >> put_device(device_link.link_dev) to make sure the device memory is no > >> longer referenced, then trigger an SRCU clean up and then in the > >> scheduled SRCU cleanup I can free struct device_link. And obviously, > >> until struct device_link is ready to be freed up, I can't drop the > >> reference to the supplier and consumer devices (as that old copy of > >> device_link could be used by some code to refer to the supplier and > >> consumer devices). > >> > >> Hope that helps explain the SRCU and device link device release dependencies. > >> > >> Also, even if this patch series is applied as is, I wonder if the > >> current overlay code has a bug related to CONFIG_DEBUG_KOBJECT_RELEASE > >> delaying the actual freeing of the device. Something to look into? > > > > I also tried enabling CONFIG_DEBUG_KOBJECT_RELEASE... with or without > > the addition of srcu_barrier() to device_links_purge(), I can't boot > > successfully when CONFIG_OF_UNITTEST=y && > > CONFIG_DEBUG_KOBJECT_RELEASE=y: there are a ton of errors that result > > from this combo. > > I'll add looking checking out booting with CONFIG_DEBUG_KOBJECT_RELEASE > enabled with CONFIG_OF_UNITTEST enabled to my todo list. > > > > > Disabling the unittests and booting with CONFIG_DEBUG_KOBJECT_RELEASE=y, > > I _do_ still see the errors mentioned in my original message when > > removing an overlay. So yeah, it does seem like there are some latent > > Just to make sure I understand clearly, you are still seeing the > messages: > > OF: ERROR: memory leak, expected refcount 1 instead of 2, of_node_get()/of_node_put() unbalanced - destroy > OF: ERROR: memory leak, expected refcount 1 instead of 2, of_node_get()/of_node_put() unbalanced - destroy > > when the overlay is removed? And if I apply patch 1/3 (the new unittest) > and also add srcu_barrier() at the end of device_links_purge() > then I will see these messages? Yes, that's correct: with CONFIG_DEBUG_KOBJECT_RELEASE=y + the new unittest, I see those errors when booting, regardless of whether the srcu_barrier is present in device_links_purge(). > Can you add a reply to this email with the patch to add srcu_barrier() at > the end of device_links_purge() so that I am testing the same thing that > you are testing? (If it makes life easier for you, you can just cut > and paste the patch into the reply, even if it results in a white > space damaged patch -- I'm assuming a one-line patch.) Sure thing: >From 87d9e27d1c10ec35a82db0c6d2b4d87ba22bbda5 Mon Sep 17 00:00:00 2001 From: Michael Auchter <michael.auchter@xxxxxx> Date: Wed, 28 Oct 2020 15:43:04 -0500 Subject: [PATCH] driver core: wait for srcu callbacks in device_links_purge Suggested-by: Saravana Kannan <saravanak@xxxxxxxxxx> Signed-off-by: Michael Auchter <michael.auchter@xxxxxx> --- drivers/base/core.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/base/core.c b/drivers/base/core.c index f90e9f77bf8c..e131cc2e7083 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -1399,6 +1399,8 @@ static void device_links_purge(struct device *dev) } device_links_write_unlock(); + + srcu_barrier(&device_links_srcu); } static u32 fw_devlink_flags = DL_FLAG_SYNC_STATE_ONLY; -- 2.25.4
