[PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Geert Uytterhoeven]

Replace the sparse 256-pointer array for looking up protocol strings by
a switch() statement to reduce kernel size.

According to bloat-o-meter, this saves 910 bytes on m68k (32-bit), and
1892 bytes on arm64 (64-bit).

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/ata/libata-eh.c | 44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 0e1ec37070d19b64..e7196fc29ff09434 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2606,21 +2606,39 @@ static void ata_eh_link_report(struct ata_link *link)
 				[DMA_TO_DEVICE]		= "out",
 				[DMA_FROM_DEVICE]	= "in",
 			};
-			static const char *prot_str[] = {
-				[ATA_PROT_UNKNOWN]	= "unknown",
-				[ATA_PROT_NODATA]	= "nodata",
-				[ATA_PROT_PIO]		= "pio",
-				[ATA_PROT_DMA]		= "dma",
-				[ATA_PROT_NCQ]		= "ncq dma",
-				[ATA_PROT_NCQ_NODATA]	= "ncq nodata",
-				[ATAPI_PROT_NODATA]	= "nodata",
-				[ATAPI_PROT_PIO]	= "pio",
-				[ATAPI_PROT_DMA]	= "dma",
-			};
+			const char *prot_str = NULL;
 
+			switch (qc->tf.protocol) {
+			case ATA_PROT_UNKNOWN:
+				prot_str = "unknown";
+				break;
+			case ATA_PROT_NODATA:
+				prot_str = "nodata";
+				break;
+			case ATA_PROT_PIO:
+				prot_str = "pio";
+				break;
+			case ATA_PROT_DMA:
+				prot_str = "dma";
+				break;
+			case ATA_PROT_NCQ:
+				prot_str = "ncq dma";
+				break;
+			case ATA_PROT_NCQ_NODATA:
+				prot_str = "ncq nodata";
+				break;
+			case ATAPI_PROT_NODATA:
+				prot_str = "nodata";
+				break;
+			case ATAPI_PROT_PIO:
+				prot_str = "pio";
+				break;
+			case ATAPI_PROT_DMA:
+				prot_str = "dma";
+				break;
+			}
 			snprintf(data_buf, sizeof(data_buf), " %s %u %s",
-				 prot_str[qc->tf.protocol], qc->nbytes,
-				 dma_str[qc->dma_dir]);
+				 prot_str, qc->nbytes, dma_str[qc->dma_dir]);
 		}
 
 		if (ata_is_atapi(qc->tf.protocol)) {
-- 
1.9.1

Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Tejun Heo]

On Mon, Jan 09, 2017 at 03:49:28PM +0100, Geert Uytterhoeven wrote:
> Replace the sparse 256-pointer array for looking up protocol strings by
> a switch() statement to reduce kernel size.
> 
> According to bloat-o-meter, this saves 910 bytes on m68k (32-bit), and
> 1892 bytes on arm64 (64-bit).
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gees, thanks for catching that.

Applied to libata/for-4.11.

Thanks.

-- 
tejun

Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Geert Uytterhoeven]

Hi Tejun,

On Mon, Jan 9, 2017 at 4:23 PM, Tejun Heo <tj@kernel.org> wrote:
> On Mon, Jan 09, 2017 at 03:49:28PM +0100, Geert Uytterhoeven wrote:
>> Replace the sparse 256-pointer array for looking up protocol strings by
>> a switch() statement to reduce kernel size.
>>
>> According to bloat-o-meter, this saves 910 bytes on m68k (32-bit), and
>> 1892 bytes on arm64 (64-bit).
>>
>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> Gees, thanks for catching that.
>
> Applied to libata/for-4.11.

Thanks!

There are two more that annoy me, but I don't know how to fix them:

ata_scsi_rbuf                                  -    4096   +4096
ata_force_param_buf                            -    4096   +4096

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

Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Tejun Heo]

Hello,

On Mon, Jan 09, 2017 at 04:40:19PM +0100, Geert Uytterhoeven wrote:
> There are two more that annoy me, but I don't know how to fix them:
> 
> ata_scsi_rbuf                                  -    4096   +4096
> ata_force_param_buf                            -    4096   +4096

ata_force_param_buf is __initdata and shouldn't really matter.
ata_scsi_rbuf, hmmm, idk.  Maybe we can allocate it dynamically when
registering the first ATA device so that systems w/o them can avoid
the wastage.

Thanks.

-- 
tejun

Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Christoph Hellwig]

On Mon, Jan 09, 2017 at 11:04:24AM -0500, Tejun Heo wrote:
> ata_force_param_buf is __initdata and shouldn't really matter.
> ata_scsi_rbuf, hmmm, idk.  Maybe we can allocate it dynamically when
> registering the first ATA device so that systems w/o them can avoid
> the wastage.

Having it global is kinda weird anyway.  But looking the code none
of the commands actually using is in the paging path, so it could
simply be replaced with a dynamic allocation in ata_scsi_rbuf_fill
for the actually needed size, which often will be very small,
or sometimes even 0.

Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Geert Uytterhoeven]

Hi Tejun,

On Mon, Jan 9, 2017 at 5:04 PM, Tejun Heo <tj@kernel.org> wrote:
> On Mon, Jan 09, 2017 at 04:40:19PM +0100, Geert Uytterhoeven wrote:
>> There are two more that annoy me, but I don't know how to fix them:
>>
>> ata_scsi_rbuf                                  -    4096   +4096
>> ata_force_param_buf                            -    4096   +4096
>
> ata_force_param_buf is __initdata and shouldn't really matter.

It mainly matters because of e.g. bootloader limitations.

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

Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Christoph Hellwig]

On Mon, Jan 09, 2017 at 08:21:43AM -0800, Christoph Hellwig wrote:
> Having it global is kinda weird anyway.  But looking the code none
> of the commands actually using is in the paging path, so it could
> simply be replaced with a dynamic allocation in ata_scsi_rbuf_fill
> for the actually needed size, which often will be very small,
> or sometimes even 0.

Prototype here, only tested with a simple mkfs.xfs and some I/O on
AHCI so far:

http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/libata-kill-ata_scsi_rbuf

Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Christoph Hellwig]

On Mon, Jan 09, 2017 at 05:30:02PM +0100, Geert Uytterhoeven wrote:
> > ata_force_param_buf is __initdata and shouldn't really matter.
> 
> It mainly matters because of e.g. bootloader limitations.

Do we need a full 4k for the force parameters?  What would a typical
command line for it look like?

Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Tejun Heo]

On Mon, Jan 09, 2017 at 09:22:07AM -0800, Christoph Hellwig wrote:
> Prototype here, only tested with a simple mkfs.xfs and some I/O on
> AHCI so far:
> 
> http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/libata-kill-ata_scsi_rbuf

Yeah, the only thing which needs completion from atomic context is
atapi commands.  If that can be done on stack, it's all good.

Thanks.

-- 
tejun

Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Tejun Heo]

Hello,

On Mon, Jan 09, 2017 at 09:27:23AM -0800, Christoph Hellwig wrote:
> On Mon, Jan 09, 2017 at 05:30:02PM +0100, Geert Uytterhoeven wrote:
> > > ata_force_param_buf is __initdata and shouldn't really matter.
> > 
> > It mainly matters because of e.g. bootloader limitations.
> 
> Do we need a full 4k for the force parameters?  What would a typical
> command line for it look like?

Maybe a couple hundreds bytes at max, but it's a bit weird to restrict
this given that it is bss, not gigantic and __initdata.  What kind of
bootloader limitations are we talking about?

Thanks.

-- 
tejun

Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Geert Uytterhoeven]

On Mon, Jan 9, 2017 at 6:31 PM, Tejun Heo <tj@kernel.org> wrote:
> On Mon, Jan 09, 2017 at 09:27:23AM -0800, Christoph Hellwig wrote:
>> On Mon, Jan 09, 2017 at 05:30:02PM +0100, Geert Uytterhoeven wrote:
>> > > ata_force_param_buf is __initdata and shouldn't really matter.
>> >
>> > It mainly matters because of e.g. bootloader limitations.
>>
>> Do we need a full 4k for the force parameters?  What would a typical
>> command line for it look like?
>
> Maybe a couple hundreds bytes at max, but it's a bit weird to restrict
> this given that it is bss, not gigantic and __initdata.  What kind of
> bootloader limitations are we talking about?

Some boot loaders start overwriting themselves or the passed DTB if the
kernel becomes too big.
If I'm not mistaken, bss is still expanded early (verified, increasing bss
can trigger the above problem).

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

Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Tejun Heo]

Hello, Geert.

On Mon, Jan 09, 2017 at 07:25:31PM +0100, Geert Uytterhoeven wrote:
> On Mon, Jan 9, 2017 at 6:31 PM, Tejun Heo <tj@kernel.org> wrote:
> > On Mon, Jan 09, 2017 at 09:27:23AM -0800, Christoph Hellwig wrote:
> >> On Mon, Jan 09, 2017 at 05:30:02PM +0100, Geert Uytterhoeven wrote:
> >> > > ata_force_param_buf is __initdata and shouldn't really matter.
> >> >
> >> > It mainly matters because of e.g. bootloader limitations.
> >>
> >> Do we need a full 4k for the force parameters?  What would a typical
> >> command line for it look like?
> >
> > Maybe a couple hundreds bytes at max, but it's a bit weird to restrict
> > this given that it is bss, not gigantic and __initdata.  What kind of
> > bootloader limitations are we talking about?
> 
> Some boot loaders start overwriting themselves or the passed DTB if the
> kernel becomes too big.
> If I'm not mistaken, bss is still expanded early (verified, increasing bss
> can trigger the above problem).

So, to avoid that, we can just kmalloc and kfree the buffer, but it
seems like a silly complication to work around bugs in some
bootloaders.  There are many places in kernel where we're liberal
about __initdata which is great.  I'm not sure complicating all those
places for a broken bootloader is a good idea.

Thanks.

-- 
tejun

Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Geert Uytterhoeven]

Hi Tejun,

On Mon, Jan 9, 2017 at 8:41 PM, Tejun Heo <tj@kernel.org> wrote:
> On Mon, Jan 09, 2017 at 07:25:31PM +0100, Geert Uytterhoeven wrote:
>> On Mon, Jan 9, 2017 at 6:31 PM, Tejun Heo <tj@kernel.org> wrote:
>> > On Mon, Jan 09, 2017 at 09:27:23AM -0800, Christoph Hellwig wrote:
>> >> On Mon, Jan 09, 2017 at 05:30:02PM +0100, Geert Uytterhoeven wrote:
>> >> > > ata_force_param_buf is __initdata and shouldn't really matter.
>> >> >
>> >> > It mainly matters because of e.g. bootloader limitations.
>> >>
>> >> Do we need a full 4k for the force parameters?  What would a typical
>> >> command line for it look like?
>> >
>> > Maybe a couple hundreds bytes at max, but it's a bit weird to restrict
>> > this given that it is bss, not gigantic and __initdata.  What kind of
>> > bootloader limitations are we talking about?
>>
>> Some boot loaders start overwriting themselves or the passed DTB if the
>> kernel becomes too big.
>> If I'm not mistaken, bss is still expanded early (verified, increasing bss
>> can trigger the above problem).
>
> So, to avoid that, we can just kmalloc and kfree the buffer, but it
> seems like a silly complication to work around bugs in some
> bootloaders.  There are many places in kernel where we're liberal
> about __initdata which is great.  I'm not sure complicating all those
> places for a broken bootloader is a good idea.

Sure. We cannot avoid that kernels (esp. multiplatform) keep on growing.

But when I see a new 4KiB-sized buffer, i'm always suspicious...
A few years ago, I caught someone miscalculating shifts, leading
to a static buffer that was 256 times larger than intended ;-)

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

Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings

[Tejun Heo]

Hello,

On Mon, Jan 09, 2017 at 09:28:12PM +0100, Geert Uytterhoeven wrote:
> > So, to avoid that, we can just kmalloc and kfree the buffer, but it
> > seems like a silly complication to work around bugs in some
> > bootloaders.  There are many places in kernel where we're liberal
> > about __initdata which is great.  I'm not sure complicating all those
> > places for a broken bootloader is a good idea.
> 
> Sure. We cannot avoid that kernels (esp. multiplatform) keep on growing.
> 
> But when I see a new 4KiB-sized buffer, i'm always suspicious...
> A few years ago, I caught someone miscalculating shifts, leading
> to a static buffer that was 256 times larger than intended ;-)

Oh, sure, things like the protocol string table are just stupid and
it's great that you caught it.  I just don't think it makes sense to
scrutinize bss __initdata.  It's not in kernel image and goes away
once the kernel boots.  It's okay to a bit liberal with them for the
sake of simplicity.

Thanks.

-- 
tejun