Hi Simon,
2015-07-20 23:16 GMT+09:00 Simon Glass <s...@chromium.org>: > Hi Masahiro, > > On 20 July 2015 at 00:21, Masahiro Yamada <yamada.masah...@socionext.com> > wrote: >> Hi Simon, Albert. >> >> >> >> 2015-07-18 23:37 GMT+09:00 Simon Glass <s...@chromium.org>: >>> +Hans >>> >>> Hi, >>> >>> On 13 July 2015 at 11:16, Albert ARIBAUD <albert.u.b...@aribaud.net> wrote: >>>> Hello Masahiro, >>>> >>>> On Mon, 13 Jul 2015 20:42:15 +0900, Masahiro Yamada >>>> <yamada.masah...@socionext.com> wrote: >>>>> 2015-07-13 19:55 GMT+09:00 Albert ARIBAUD <albert.u.b...@aribaud.net>: >>>>> > Hello Masahiro, >>>>> > >>>>> > On Mon, 13 Jul 2015 16:39:45 +0900, Masahiro Yamada >>>>> > <yamada.masah...@socionext.com> wrote: >>>>> >> Hi Albert, >>>>> >> >>>>> >> >>>>> >> 2015-07-13 15:51 GMT+09:00 Albert ARIBAUD <albert.u.b...@aribaud.net>: >>>>> >> > Hello Masahiro, >>>>> >> > >>>>> >> > On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada >>>>> >> > <yamada.masah...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
