| Message ID | 20170228041857.13292-3-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 27, 2017 at 08:18:56PM -0800, Dmitry Torokhov wrote: > Lists of spi_board_info entries registered with spi_register_board_info() > can be quite long; instead of forcing memory allocator find contagious Do you have numbers on that?
On Tue, Feb 28, 2017 at 09:16:50AM +0000, Mark Brown wrote: > On Mon, Feb 27, 2017 at 08:18:56PM -0800, Dmitry Torokhov wrote: > > Lists of spi_board_info entries registered with spi_register_board_info() > > can be quite long; instead of forcing memory allocator find contagious > > Do you have numbers on that? Hm, so the largest array seems to be in arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above page, but still could be packed I think. If we decide that we want to keep single chunk I'll just change the allocation to kcalloc. Let me know. We should probably redo patch #1 to avoid allocating empty property sets anyway. Thanks.
On Tue, Feb 28, 2017 at 10:24:17AM -0800, Dmitry Torokhov wrote: > On Tue, Feb 28, 2017 at 09:16:50AM +0000, Mark Brown wrote: > > On Mon, Feb 27, 2017 at 08:18:56PM -0800, Dmitry Torokhov wrote: > > > Lists of spi_board_info entries registered with spi_register_board_info() > > > can be quite long; instead of forcing memory allocator find contagious > > Do you have numbers on that? > Hm, so the largest array seems to be in > arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new > board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above > page, but still could be packed I think. Oh wow, that's impressively large. Still not sure the optimization is particularly worth it though, it's small change in the grand scheme of things. OTOH it's a small change. > If we decide that we want to keep single chunk I'll just change the > allocation to kcalloc. Let me know. I'd be inclined to do that because it requires less thinking about the value of what should be a very small optimization either way but whatever :) > We should probably redo patch #1 to avoid allocating empty property sets > anyway. Yeah.
On Tue, Feb 28, 2017 at 7:54 PM, Mark Brown <broonie@kernel.org> wrote: > On Tue, Feb 28, 2017 at 10:24:17AM -0800, Dmitry Torokhov wrote: >> On Tue, Feb 28, 2017 at 09:16:50AM +0000, Mark Brown wrote: >> > On Mon, Feb 27, 2017 at 08:18:56PM -0800, Dmitry Torokhov wrote: >> > > Lists of spi_board_info entries registered with spi_register_board_info() >> > > can be quite long; instead of forcing memory allocator find contagious > >> > Do you have numbers on that? > >> Hm, so the largest array seems to be in >> arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new >> board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above >> page, but still could be packed I think. > > Oh wow, that's impressively large. Still not sure the optimization is > particularly worth it though, it's small change in the grand scheme of > things. OTOH it's a small change. Given this is done during early boot, what's the probability of not having sufficient contiguous memory? >> If we decide that we want to keep single chunk I'll just change the >> allocation to kcalloc. Let me know. > > I'd be inclined to do that because it requires less thinking about the > value of what should be a very small optimization either way but > whatever :) Tada... commit f9bdb7fdd2cac17bdc9c344b6036e6939fa087cd Author: Markus Elfring <elfring@users.sourceforge.net> Date: Fri Jan 13 12:28:04 2017 +0100 spi: Use kcalloc() in spi_register_board_info() Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/28/2017 07:54 PM, Mark Brown wrote: > On Tue, Feb 28, 2017 at 10:24:17AM -0800, Dmitry Torokhov wrote: >> On Tue, Feb 28, 2017 at 09:16:50AM +0000, Mark Brown wrote: >>> On Mon, Feb 27, 2017 at 08:18:56PM -0800, Dmitry Torokhov wrote: >>>> Lists of spi_board_info entries registered with spi_register_board_info() >>>> can be quite long; instead of forcing memory allocator find contagious > >>> Do you have numbers on that? > >> Hm, so the largest array seems to be in >> arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new >> board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above >> page, but still could be packed I think. > > Oh wow, that's impressively large. Still not sure the optimization is > particularly worth it though, it's small change in the grand scheme of > things. OTOH it's a small change. The Blackfin machine files shouldn't be used as valid example. While in theory it is possible to build a kernel with that many entries nobody is ever going to do that. What the Blackfin machine files do is basically a poor man's overlays from the days before overlays existed. Most of the entries in the table use the same chip-select pins, it wouldn't be possible to use use a setup where more than two or three of the entries is enabled at the same time. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 28, 2017 at 09:15:12PM +0100, Geert Uytterhoeven wrote: > On Tue, Feb 28, 2017 at 7:54 PM, Mark Brown <broonie@kernel.org> wrote: > > On Tue, Feb 28, 2017 at 10:24:17AM -0800, Dmitry Torokhov wrote: > >> On Tue, Feb 28, 2017 at 09:16:50AM +0000, Mark Brown wrote: > >> > On Mon, Feb 27, 2017 at 08:18:56PM -0800, Dmitry Torokhov wrote: > >> > > Lists of spi_board_info entries registered with spi_register_board_info() > >> > > can be quite long; instead of forcing memory allocator find contagious > > > >> > Do you have numbers on that? > > > >> Hm, so the largest array seems to be in > >> arch/blackfin/mach-bf537/boards/stamp.c at max of 43 entries. The new > >> board info is ether 60 or 72 bytes, so we get 2 or 3K table. Not above > >> page, but still could be packed I think. > > > > Oh wow, that's impressively large. Still not sure the optimization is > > particularly worth it though, it's small change in the grand scheme of > > things. OTOH it's a small change. > > Given this is done during early boot, what's the probability of not having > sufficient contiguous memory? > > >> If we decide that we want to keep single chunk I'll just change the > >> allocation to kcalloc. Let me know. > > > > I'd be inclined to do that because it requires less thinking about the > > value of what should be a very small optimization either way but > > whatever :) > > Tada... > > commit f9bdb7fdd2cac17bdc9c344b6036e6939fa087cd > Author: Markus Elfring <elfring@users.sourceforge.net> > Date: Fri Jan 13 12:28:04 2017 +0100 > > spi: Use kcalloc() in spi_register_board_info() Or even kmalloc_array() as zeroing out memory is not needed. But I'll let Mark sort it out and drop my patch #2. Thanks.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index b948d8cdbace..1b66e0497327 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -686,14 +686,14 @@ int spi_register_board_info(struct spi_board_info const *info, unsigned n) if (!n) return -EINVAL; - bi = kzalloc(n * sizeof(*bi), GFP_KERNEL); - if (!bi) - return -ENOMEM; - - for (i = 0; i < n; i++, bi++, info++) { + for (i = 0; i < n; i++, info++) { struct spi_master *master; - memcpy(&bi->board_info, info, sizeof(*info)); + bi = kmalloc(sizeof(*bi), GFP_KERNEL); + if (!bi) + return -ENOMEM; + + bi->board_info = *info; bi->board_info.properties = property_entries_dup(info->properties); if (IS_ERR(bi->board_info.properties))
Lists of spi_board_info entries registered with spi_register_board_info() can be quite long; instead of forcing memory allocator find contagious chunk of memory, let;s allocate them one-by-one, so they can be packed as needed. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/spi/spi.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)