Hi Masahiro,
On 20 July 2015 at 00:21, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Simon, Albert.
2015-07-18 23:37 GMT+09:00 Simon Glass sjg@chromium.org:
+Hans
Hi,
On 13 July 2015 at 11:16, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hello Masahiro,
On Mon, 13 Jul 2015 20:42:15 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
2015-07-13 19:55 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net:
Hello Masahiro,
On Mon, 13 Jul 2015 16:39:45 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Albert,
2015-07-13 15:51 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net: > Hello Masahiro, > > On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada > yamada.masahiro@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:
- 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.
- Drop the use of devres to remove()/unbind() devices. The core DM
code can stick with its existing manual free() calls.
devres_head should only exist in struct device when CONFIG_DEVRES is set.
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!
- 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.
Regards, Simon