[U-Boot] [PATCH v2 00/14] Devres (Managed Device Resource) for U-Boot

[Masahiro Yamada]

Hi Simon,



2015-07-20 23:16 GMT+09:00 Simon Glass <sjg at chromium.org>:
> Hi Masahiro,
>
> On 20 July 2015 at 00:21, Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
>> Hi Simon, Albert.
>>
>>
>>
>> 2015-07-18 23:37 GMT+09:00 Simon Glass <sjg at chromium.org>:
>>> +Hans
>>>
>>> Hi,
>>>
>>> On 13 July 2015 at 11:16, Albert ARIBAUD <albert.u.boot at aribaud.net> wrote:
>>>> Hello Masahiro,
>>>>
>>>> On Mon, 13 Jul 2015 20:42:15 +0900, Masahiro Yamada
>>>> <yamada.masahiro at socionext.com> wrote:
>>>>> 2015-07-13 19:55 GMT+09:00 Albert ARIBAUD <albert.u.boot at aribaud.net>:
>>>>> > Hello Masahiro,
>>>>> >
>>>>> > On Mon, 13 Jul 2015 16:39:45 +0900, Masahiro Yamada
>>>>> > <yamada.masahiro at socionext.com> wrote:
>>>>> >> Hi Albert,
>>>>> >>
>>>>> >>
>>>>> >> 2015-07-13 15:51 GMT+09:00 Albert ARIBAUD <albert.u.boot at aribaud.net>:
>>>>> >> > Hello Masahiro,
>>>>> >> >
>>>>> >> > On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada
>>>>> >> > <yamada.masahiro at socionext.com> wrote:
>>>>> >> >>
>>>>> >> >> Please refer to the commit message of 06/14
>>>>> >> >> for what this series wants to do.
>>>>> >> >
>>>>> >> > Remark: you could use "Series-notes:" in 6/14 to have patman directly
>>>>> >> > include said notes here instead of referring the reader to the patch.
>>>>> >> >
>>>>> >> >> Masahiro Yamada (14):
>>>>> >> >>   x86: delete unneeded declarations of disable_irq() and enable_irq()
>>>>> >> >>   linux_compat: remove cpu_relax() define
>>>>> >> >>   linux_compat: move vzalloc() to header file as an inline function
>>>>> >> >>   linux_compat: handle __GFP_ZERO in kmalloc()
>>>>> >> >>   dm: add DM_FLAG_BOUND flag
>>>>> >> >>   devres: introduce Devres (Managed Device Resource) framework
>>>>> >> >>   devres: add devm_kmalloc() and friends (managed memory allocators)
>>>>> >> >>   dm: refactor device_bind() and device_unbind() with devm_kzalloc()
>>>>> >> >>   dm: merge fail_alloc labels
>>>>> >> >>   linux_compat: introduce GFP_DMA flag for kmalloc()
>>>>> >> >>   dm: refactor device_probe() and device_remove() with devm_kzalloc()
>>>>> >> >>   devres: add debug command to dump device resources
>>>>> >> >>   dm: remove redundant CONFIG_DM from driver/core/Makefile
>>>>> >> >>   devres: compile Devres iif CONFIG_DM_DEVICE_REMOVE is enabled
>>>>> >> >
>>>>> >> > I am still unsure why this is necessary. I had a discussion on the
>>>>> >> > list with Simon, see last message here:
>>>>> >> >
>>>>> >> > <https://www.mail-archive.com/u-boot@lists.denx.de/msg177031.html>
>>>>> >> >
>>>>> >> > Unless I'm mistaken, the only case where we actually have a leak that
>>>>> >> > this series would fix is in some cases of binding USB devices / drivers
>>>>> >> > multiple times, and even then, it would take a considerable amount of
>>>>> >> > repeated bindings before U-Boot could become unable to boot a payload;
>>>>> >> > a scenario that I find unlikely.
>>>>> >> >
>>>>> >> > I do understand the general goal of fixing bugs when we ind them; but I
>>>>> >> > question the global benefit of fixing this specific leak bug by adding a
>>>>> >> > whole new feature with a lot of code to U-Boot, as opposed to fixing
>>>>> >> > it in a more ad hoc way with much less less code volume and complexity.
>>>>> >>
>>>>> >>
>>>>> >> You have a point.
>>>>> >>
>>>>> >> What we really want to avoid is to make low-level drivers too complicated
>>>>> >> by leaving the correct memory management to each of them.
>>>>> >>
>>>>> >> After all, there turned out to be two options to solve it.
>>>>> >>
>>>>> >>   [1] Simon's driver model:  move allocating/freeing memory to the driver core
>>>>> >>                              by having only the needed memory size
>>>>> >> specified in each driver
>>>>> >>   [2] Devres: we still have to allocate memory in each driver, but we
>>>>> >> need not free it explicitly,
>>>>> >>                making our driver work much easier
>>>>> >>
>>>>> >>
>>>>> >> [1] and [2] are completely differently approach,
>>>>> >> but what they solve is the same:  easier memory (resource) management
>>>>> >> without leak.
>>>>> >
>>>>> > Understood.
>>>>> >
>>>>> > IIUC, this adds a new leak scenario beside the multiple inits one such
>>>>> > as for USB, but this new scenarion which is in failure paths where
>>>>> > already allocated memory must be released, seems to me no more critical
>>>>> > than the one I'd discussed with Simon, i.e., I'm not convinced we need
>>>>> > a fix, and I'm not convinced we need a general memory management
>>>>> > feature for that -- which does not mean I can't be convinced, though; I
>>>>> > just need (more) convincing arguments (than I can currently read).
>>>>>
>>>>> BTW, I am not following the USB-discussion between Simon and you.
>>>>> I have not checked the USB changes recently,
>>>>> so I am not familiar enough with it to add my comment.
>>>>>
>>>> It was not actually a USB discussion. It was a discussion within v1 of
>>>> this patch series. I'd asked when exactly there could be leaks in our
>>>> current driver user scenarios and Simon said there was no leak case
>>>> except in USB which could be bound several times in U-Boot between
>>>> power-on and OS boot.
>>>>
>>>>> >> The only problem I see in [1] is that it is not controllable at run-time.
>>>>> >> The memory size for the auto-allocation must be specified at the compile time.
>>>>> >
>>>>> > How in practice is that a problem?
>>>>>
>>>>> At first, I thought (or expected) that .priv_auto_alloc_size and friends
>>>>> were generic enough to cover all the cases, but they were not.
>>>>
>>>> I'm afraid I am still not seeing the problem.
>>>>
>>>>> >> So, we need calloc() and free() in some low-level drivers.
>>>>> >> Simon might say they are only a few "exceptions",
>>>>> >> (my impression is I often hear the logic such as "it is not a problem
>>>>> >> because we do not have many.")
>>>>> >> Anyway, we had already lost the consistency as for memory allocation.
>>>>> >
>>>>> > Not sure I understand that last sentence. Which consistency exactly
>>>>> > have we lost?
>>>>>
>>>>> When I write drivers for Linux, I usually allocate memory for driver data
>>>>> with devm_kzalloc().
>>>>>
>>>>> But, in U-boot, sometimes we specify .priv_auto_alloc_size,
>>>>> and sometimes we call calloc() and free().
>>>>>
>>>>> This is what I called lost-consistency.
>>>>
>>>> Ok, now I get this point.
>>>>
>>>>> >> I imagined if [2] had been supported earlier, we would not have needed [1].
>>>>> >> (at least, [2] seems more flexible to me.)
>>>>> >>
>>>>> >> We already have many DM-based drivers, and I think we can live with [1]
>>>>> >> despite some exceptional drivers allocating memory on their own.
>>>>> >>
>>>>> >> So, if Simon (and other developers) does not feel comfortable with this series,
>>>>> >> I do not mind discarding it.
>>>>> >
>>>>> > Note that I'm not saying your patch series does not solve the issue of
>>>>> > leaks. What I'm saying is i) do we need to solve this issue, and ii) if we
>>>>> > do, is your series the best option ?
>>>>>
>>>>> Sorry, if I am misunderstanding your questions.
>>>>>
>>>>> If you are talking about the generic program guideline, I think
>>>>> leaks should be eliminated and this series should be helpful
>>>>> (but I wouldn't say as far as it is the best).
>>>>>
>>>>> If you are asking if this series is the best for solving some particular issues,
>>>>> sorry,  I can not comment on what I am not familiar with.
>>>>
>>>> Let me recap :) as a series of simple yes/no sentences.
>>>>
>>>> - Sometimes in driver code there are memory leaks.
>>>>
>>>> - These leaks are bugs.
>>>>
>>>> - These leaks do not prevent U-Boot from functioning properly.
>>>>
>>>> - There are two methods to eliminate these leaks: Simon's method of
>>>>   allocating / freeing driver/device memory outside driver code per se,
>>>>   and your proposed method of dynamically tracking memory allocated by
>>>>   a driver / device, and freeing it when the driver gets 'unloaded' --
>>>>   akin to freeing a process' resources when it terminates.
>>>>
>>>> - Each method has advantages and disadvantages.
>>>>
>>>> My opinion for now is that:
>>>>
>>>> - We do not /need/ to fix the leaks, we /would like/ to.
>>>>
>>>> - since we don't /need/ to fix the leaks, we can afford to be
>>>>   picky about how we will fix them, or even whether we will fix them
>>>>   at all if no solution pleases us.
>>>>
>>>> - 'being picky' means we should consider the pros and cons of available
>>>>   methods, not only wrt the fix itself, but more generally too: Does it
>>>>   fix other issues? Does it hinder or encourage best practices? how much
>>>>   source code does it bring in? etc.
>>>>
>>>> Right now, I'm not even certain we have a problem at all, in the sense
>>>> that I don't recall seeing reports of a target failing to boot because
>>>> of memory exhaustion in U-Boot -- which means that, to me, the 'pros'
>>>> of any leak fix solution would start thin since they would solve a
>>>> non-problem, or more precisely, a not-really-a-problem, and I fail to
>>>> see the other pros (OTOH, the cons are not /that/ many either).
>>>>
>>>> But I might be mistaken, so I'm asking for actual scenarios where a
>>>> target did have problems doing its job due to memory allocation issues,
>>>> and scenarios for other pros as well, and comments from anyone who
>>>> would have a good idea of the cons and pros.
>>>
>>> It's a valuable point of view. My instinct is often to bring things in
>>> and expand the platform. But there needs to be a strong benefit.
>>>
>>> As things stand I am also unsure of this. Driver model was designed to
>>> put memory allocation inside the core code (driver/core/device.c).
>>> Very few devices allocate memory at present () and perhaps even fewer
>>> need to. Here's a list of:
>>>
>>> $ grep "alloc(" `git grep -l  U_BOOT_DRIVER`
>>> arch/x86/cpu/ivybridge/bd82x6x.c: cpu = calloc(1, sizeof(*cpu));
>>> drivers/gpio/mxc_gpio.c: plat = calloc(1, sizeof(*plat));
>>> drivers/gpio/s5p_gpio.c: plat = calloc(1, sizeof(*plat));
>>> drivers/gpio/sandbox.c: dev->priv = calloc(sizeof(struct gpio_state),
>>> uc_priv->gpio_count);
>>> drivers/gpio/sunxi_gpio.c: name = malloc(3);
>>> drivers/gpio/sunxi_gpio.c: plat = calloc(1, sizeof(*plat));
>>> drivers/gpio/tegra_gpio.c: name = malloc(3);
>>> drivers/gpio/tegra_gpio.c: plat = calloc(1, sizeof(*plat));
>>> drivers/gpio/vybrid_gpio.c: plat = calloc(1, sizeof(*plat));
>>> drivers/i2c/i2c-uclass.c: * Use the stack for small messages, malloc()
>>> for larger ones. We
>>> drivers/i2c/i2c-uclass.c: buf = malloc(I2C_MAX_OFFSET_LEN + len);
>>> drivers/misc/cros_ec_sandbox.c: ec->flash_data = os_malloc(len);
>>> drivers/misc/cros_ec_sandbox.c: ec->matrix = calloc(ec->matrix_count,
>>> sizeof(*ec->matrix));
>>> drivers/misc/cros_ec_sandbox.c: ec->flash_data = os_malloc(ec->flash_data_len);
>>> drivers/misc/i2c_eeprom_emul.c: priv->data = calloc(1, plat->size);
>>> drivers/mtd/spi/sf_probe.c: flash = calloc(1, sizeof(*flash));
>>> drivers/net/designware.c: struct mii_dev *bus = mdio_alloc();
>>> drivers/net/designware.c: dev = (struct eth_device *)
>>> malloc(sizeof(struct eth_device));
>>> drivers/net/sunxi_emac.c: priv->bus = mdio_alloc();
>>> drivers/spi/sandbox_spi.c: tx = malloc(bytes);
>>> drivers/spi/sandbox_spi.c: rx = malloc(bytes);
>>> test/dm/test-driver.c: dev->priv = calloc(1, sizeof(struct dm_test_priv));
>>>
>>> Most of those are temporary allocations are unnecessary but some could
>>> use devres.
>>>
>>> The v2 series solves the SPL size issue, except that with Han's USB
>>> changes we have to device DM_REMOVE when we use USB. I am seriously
>>> reconsidering that as a limitation that might be best avoided.
>>>
>>> But we now require devres as a core feature - this pushes up the
>>> overhead of driver rmodel. I really like this patch:
>>>
>>> http://patchwork.ozlabs.org/patch/494216/
>>>
>>> but I don't think it goes far enough.
>>>
>>> I'd like to figure this out before moving to v3. Here is my proposal:
>>>
>>> 1. Add CONFIG_DEVRES as an option which can be enabled if wanted, but
>>> is not required for things to work.
>>
>>
>> This is a boot-loader, so it is a quite rare scenario to attach and detach
>> drivers over and over again.
>> Memory exhaustion would never happen (at least in normal usage) even
>> if memory is never freed.
>>
>> Things always work well, in other words, there is no use-case
>> where precise resource management is our requirement.
>>
>> So, CONFIG_DEVRES can be optional.
>
> I agree - the main thing I can think of is USB but even then it would
> mostly be a human fiddling with it. A normal device boot sequence
> would not rescan the bus multiple times.
>
>>
>> Memory may leak if CONFIG_DEVRES is disabled, but that is OK.
>> That is not what we care about very much.
>>
>> This is what I understood so far.
>>
>> If so, CONFIG_DEVRES and CONFIG_DM_REMOVE eventually have similar concept:
>> We prepared these features just in case, but we actually do not need them,
>> so they are the first things we want to disable in memory-limited cases.
>>
>> We had already disabled CONFIG_DM_REMOVE where we really want to save
>> memory footprint, such as SPL.
>> I do not think we will get further benefit by adding CONFIG_DEVRES here.
>>
>>
>> Also, another similar concept we have is, malloc_simple, where
>> free() does nothing, we cannot retrieve freed memory.
>>
>> I think too many choices will result in making things messy.
>>
>
> We have found a case where we want CONFIG_DM_REMOVE in normal
> operation - shutting down USB before booting the OS.
>
> Extra options introduce extra cases, but they are all different:
>
> - malloc_simple - reduced code size for supporting malloc()
> - DM_REMOVE - drops all remove/unbind code (actually it doesn't drop
> it in drivers, which could be fixed)
> - DEVRES - supports manual memory allocation in drivers with automatic free
>
> So I don't see any conflict here. Also I like the memory allocation
> improvements you have added.
>
>>
>>
>>> 2. Drop the use of devres to remove()/unbind() devices. The core DM
>>> code can stick with its existing manual free() calls.
>>>
>>> 3. devres_head should only exist in struct device when CONFIG_DEVRES is set.
>>>
>>> 4. Convert over a driver to use devres in sandbox and enable it. One
>>> option would be cros_ec_sandbox. It reads the device tree key map and
>>> it is variable size so we can't use the automatic memory allocation.
>>> Need to add a remove() method!
>>>
>>> 5. See how much use devres gets over the next year. We haven't lost
>>> any efficiency and we gain a useful feature to track device memory
>>> allocation.
>>>
>>> Masahiro, Albert what do you think?
>>>
>>> Regards,
>>> Simon
>>
>>
>> From our discussion so far, my frank feeling is, this series is unwelcome.
>>
>> Rather than pulling in what we are unsure about the necessity,
>> won't it be better to defer it to see if it is really useful or not?
>> (i.e. how many drivers want to do malloc on their own.)
>>
>> Anyway, we can dig it in Patchwork anytime we find it necessary.
>
> If we apply it then we can tell people to always use it when
> allocating memory manually in a driver. We can go through and make
> sure all existing cases use it.
>
> Then we have options in the future when we need to enable it. If we
> don't apply it, we build up code that would need to be refactored
> later in this case.
>
> Also devres is a concept understood in the kernel, and if we can bring
> it in without adding mandatory overhead, that seems like a win to me.
>
> My preference is to apply it, but as an option. So long as sandbox
> uses it, this will ensure adequate test coverage. BTW we already have
> memory leak tests but I'm not sure if they will be enough to test
> devres.


OK, I will try v3, making it optional with CONFIG_DEVRES.



-- 
Best Regards
Masahiro Yamada