Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1589166 > unrolled thread

[PATCH v2 2/3] spi: allocate spi_board_info entries one by one

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 2/3] spi: allocate spi_board_info entries one by one Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-28 05:30 +0100
    Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one Mark Brown <broonie@kernel.org> - 2017-02-28 10:20 +0100
      Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one Mark Brown <broonie@kernel.org> - 2017-02-28 21:10 +0100
        Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-28 21:20 +0100
          Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-28 23:20 +0100
        Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one Lars-Peter Clausen <lars@metafoo.de> - 2017-02-28 22:00 +0100
      Re: [PATCH v2 2/3] spi: allocate spi_board_info entries one by one Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-03-01 03:20 +0100

#1589166 — [PATCH v2 2/3] spi: allocate spi_board_info entries one by one

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(-)

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))
-- 
2.11.0.483.g087da7b7c-goog

[toc] | [next] | [standalone]

#1589306

[Multipart message — attachments visible in raw view] — view raw

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?

[toc] | [prev] | [next] | [standalone]

#1589738

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1589747

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

[toc] | [prev] | [next] | [standalone]

#1589838

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.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1589767

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.

[toc] | [prev] | [next] | [standalone]

#1589947

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.

-- 
Dmitry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web