[PATCH] scsi: eata: drop VLA in reorder()

[PATCH] scsi: eata: drop VLA in reorder()

[Tobin C. Harding]

Adding kernel newbies to CC because I pose a few noob questions :)
Adding Linus to CC because I quoted him.

On Sun, Mar 11, 2018 at 10:06:58PM +0100, Salvatore Mesoraca wrote:
> n_ready will always be less than or equal to MAX_MAILBOXES.
> So we avoid a VLA[1] and use fixed-length arrays instead.
> 
> [1] https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
> ---
>  drivers/scsi/eata.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
> index 6501c33..202cd17 100644
> --- a/drivers/scsi/eata.c
> +++ b/drivers/scsi/eata.c
> @@ -2096,7 +2096,7 @@ static int reorder(struct hostdata *ha, unsigned long cursec,
>  	unsigned int k, n;
>  	unsigned int rev = 0, s = 1, r = 1;
>  	unsigned int input_only = 1, overlap = 0;
> -	unsigned long sl[n_ready], pl[n_ready], ll[n_ready];
> +	unsigned long sl[MAX_MAILBOXES], pl[MAX_MAILBOXES], ll[MAX_MAILBOXES];

I think we are going to see a recurring theme here.  MAX_MAILBOXES==64
so this patch adds 1536 bytes to the stack on a 64 bit machine or 768
bytes on a 32 bit machine.  Linus already commented on another VLA
removal patch that 768 was a lot of stack space.  That comment did,
however say 'deep in some transfer call chain'.  I don't know what a
'transfer call chain' (the transfer bit) is but is there some heuristic
we can use to know how deep is deep?  Or more to the point, is there some
heuristic we can use to know what is an acceptable amount of stack space
to use?

As far as this patch is concerned wouldn't a kmalloc (with GFP_ATOMIC)
be ok?  We are in an interrupt handler, can we assume that since IO has
just occurred that the IO will be so slow comparatively that a memory
allocation will be quick.  (assuming IO since eata.c only requests a
single irq line.)


thanks,
Tobin.

[PATCH] scsi: eata: drop VLA in reorder()

[valdis.kletnieks at vt.edu]

On Mon, 12 Mar 2018 14:08:34 +1100, "Tobin C. Harding" said:

> removal patch that 768 was a lot of stack space.  That comment did,
> however say 'deep in some transfer call chain'.  I don't know what a
> 'transfer call chain' (the transfer bit) is but is there some heuristic
> we can use to know how deep is deep?  Or more to the point, is there some
> heuristic we can use to know what is an acceptable amount of stack space
> to use?

The canonical "why we put kernel stacks on a diet" configuration:

Imagine a bunch of ISCSI targets - with IPSec wrapping the connection.
Arranged into a software RAID5. With LVM. With encryption on the LV.  With XFS
on the encrypted LV.  And then the in-kernel sharing it out over NFS. With
more IPSec wrapping the  NFS TCP connection.

Oh, and I/O interrupts, just for fun.  And most of all of that has to fit their *entire*
stack chain into 2 4K pages.  Suddenly, that 768 bytes is taking close to 10% of
*all* of the stack that all of that call chain has to share.

And I see that patch is against scsi/eata.c - which means it can plausibly end up
sharing that stack scenario above starting at 'software raid5'.

(For bonus points, the alert reader is invited to figure out which stack each of the
above actually ends up on.  No, it isn't split across enough stacks that taking
768 bytes out of any of them is acceptable.. :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 486 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180312/0cf62773/attachment.sig>

[PATCH] scsi: eata: drop VLA in reorder()

[Salvatore Mesoraca]

2018-03-12 4:08 GMT+01:00 Tobin C. Harding <tobin@apporbit.com>:
> Adding kernel newbies to CC because I pose a few noob questions :)
> Adding Linus to CC because I quoted him.
>
> On Sun, Mar 11, 2018 at 10:06:58PM +0100, Salvatore Mesoraca wrote:
>> n_ready will always be less than or equal to MAX_MAILBOXES.
>> So we avoid a VLA[1] and use fixed-length arrays instead.
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
>> ---
>>  drivers/scsi/eata.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
>> index 6501c33..202cd17 100644
>> --- a/drivers/scsi/eata.c
>> +++ b/drivers/scsi/eata.c
>> @@ -2096,7 +2096,7 @@ static int reorder(struct hostdata *ha, unsigned long cursec,
>>       unsigned int k, n;
>>       unsigned int rev = 0, s = 1, r = 1;
>>       unsigned int input_only = 1, overlap = 0;
>> -     unsigned long sl[n_ready], pl[n_ready], ll[n_ready];
>> +     unsigned long sl[MAX_MAILBOXES], pl[MAX_MAILBOXES], ll[MAX_MAILBOXES];
>
> I think we are going to see a recurring theme here.  MAX_MAILBOXES==64
> so this patch adds 1536 bytes to the stack on a 64 bit machine or 768
> bytes on a 32 bit machine.  Linus already commented on another VLA
> removal patch that 768 was a lot of stack space.  That comment did,
> however say 'deep in some transfer call chain'.  I don't know what a
> 'transfer call chain' (the transfer bit) is but is there some heuristic
> we can use to know how deep is deep?  Or more to the point, is there some
> heuristic we can use to know what is an acceptable amount of stack space
> to use?
>
> As far as this patch is concerned wouldn't a kmalloc (with GFP_ATOMIC)
> be ok?  We are in an interrupt handler, can we assume that since IO has
> just occurred that the IO will be so slow comparatively that a memory
> allocation will be quick.  (assuming IO since eata.c only requests a
> single irq line.)

Yes, I think you are right. I'll change it in v2.
Thank you very much,

Salvatore

[PATCH] scsi: eata: drop VLA in reorder()

[Linus Torvalds]

On Sun, Mar 11, 2018 at 8:08 PM, Tobin C. Harding <tobin@apporbit.com> wrote:
>
> I think we are going to see a recurring theme here.  MAX_MAILBOXES==64
> so this patch adds 1536 bytes to the stack on a 64 bit machine or 768
> bytes on a 32 bit machine.

Yeah, that's a bit excessive. It probably works, but one or two of
those allocations will make the kernel stack really tight, so in
general I really would suggest using kmalloc() instead, or figuring
out some way to simply shrink the data structures.

That said, I wonder if the solution to this particular driver is
"delete it". Because the hardware is truly ancient and nobody sane
would use it any more.

The last patch that seemed to come from an actual _user_ finding a
problem was in 2008 (commit 20c09df7eb9c: "[SCSI] eata: fix the data
buffer accessors conversion regression"). And even then it apparently
took a year for people to have noticed the breakage.

But because the person who reported that problem is still around, I'll
just add him to the cc, just in case.

Arthur Marsh, you have the dubious honor and distinction of being the
only person to have apparently used that driver in the last ten years.
Do you still have hardware using that? Because maybe it's really time
to retire that driver.

                   Linus

[PATCH] scsi: eata: drop VLA in reorder()

[Arthur Marsh]

Linus Torvalds wrote on 13/03/18 05:15:
> On Sun, Mar 11, 2018 at 8:08 PM, Tobin C. Harding <tobin@apporbit.com> wrote:
>>
>> I think we are going to see a recurring theme here.  MAX_MAILBOXES==64
>> so this patch adds 1536 bytes to the stack on a 64 bit machine or 768
>> bytes on a 32 bit machine.
> 
> Yeah, that's a bit excessive. It probably works, but one or two of
> those allocations will make the kernel stack really tight, so in
> general I really would suggest using kmalloc() instead, or figuring
> out some way to simply shrink the data structures.
> 
> That said, I wonder if the solution to this particular driver is
> "delete it". Because the hardware is truly ancient and nobody sane
> would use it any more.
> 
> The last patch that seemed to come from an actual _user_ finding a
> problem was in 2008 (commit 20c09df7eb9c: "[SCSI] eata: fix the data
> buffer accessors conversion regression"). And even then it apparently
> took a year for people to have noticed the breakage.
> 
> But because the person who reported that problem is still around, I'll
> just add him to the cc, just in case.
> 
> Arthur Marsh, you have the dubious honor and distinction of being the
> only person to have apparently used that driver in the last ten years.
> Do you still have hardware using that? Because maybe it's really time
> to retire that driver.
> 
>                     Linus
> 

Hi Linus and maintainers, thanks for the courtesy email and all the help 
with the driver.

I am unable to make use of the driver any more due to failed hardware.

The DPT2044W SCSI controller and the IBM disk from May 1998 last 
officially ran on 7 August 2017. I was had previously been able to get 
the data off it and disconnected the controller and disk following 
recurring problems with booting.

Aug  7 16:40:24 localhost kernel: [  105.098705] sd 0:0:6:0: [sda] 
Synchronizing SCSI cache
Aug  7 16:40:24 localhost kernel: [  105.233166] EATA0: IRQ 11 mapped to 
IO-APIC IRQ 18.
Aug  7 16:40:24 localhost kernel: [  105.233475] EATA/DMA 2.0x: 
Copyright (C) 1994-2003 Dario Ballabio.
Aug  7 16:40:24 localhost kernel: [  105.233485] EATA config options -> 
tm:1, lc:y, mq:16, rs:y, et:n, ip:n, ep:n, pp:y.
Aug  7 16:40:24 localhost kernel: [  105.233492] EATA0: 2.0C, PCI 
0x9010, IRQ 18, BMST, SG 122, MB 64.
Aug  7 16:40:24 localhost kernel: [  105.233499] EATA0: wide SCSI 
support enabled, max_id 16, max_lun 8.
Aug  7 16:40:24 localhost kernel: [  105.233505] EATA0: SCSI channel 0 
enabled, host target ID 7.
Aug  7 16:40:24 localhost kernel: [  105.233521] scsi host0: EATA/DMA 
2.0x rev. 8.10.00

Arthur Marsh.

[PATCH] scsi: eata: drop VLA in reorder()

[Martin K. Petersen]

Linus,

> That said, I wonder if the solution to this particular driver is
> "delete it". Because the hardware is truly ancient and nobody sane
> would use it any more.

I'm not aware of anybody actively using these anymore. They are
mid-nineties vintage with an M68K processor onboard. I ran a couple of
these when they were new but haven't had a working board in probably a
decade.

No objections to Salvatore's patch but I have a slight affinity for
retiring unused code over patching it. So unless there are objections...

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH] scsi: eata: drop VLA in reorder()

[Christoph Hellwig]

On Mon, Mar 12, 2018 at 10:35:36PM -0400, Martin K. Petersen wrote:
> No objections to Salvatore's patch but I have a slight affinity for
> retiring unused code over patching it. So unless there are objections...

Lets kill it.  And the not DMA capable eata_pio driver with it for
good riddance.

> 
> -- 
> Martin K. Petersen	Oracle Linux Engineering
---end quoted text---

[PATCH] scsi: eata: drop VLA in reorder()

[Salvatore Mesoraca]

2018-03-13 10:05 GMT+01:00 Christoph Hellwig <hch@infradead.org>:
> On Mon, Mar 12, 2018 at 10:35:36PM -0400, Martin K. Petersen wrote:
>> No objections to Salvatore's patch but I have a slight affinity for
>> retiring unused code over patching it. So unless there are objections...
>
> Lets kill it.  And the not DMA capable eata_pio driver with it for
> good riddance.

Good, I'll send a patch to remove eata & friends.
Thank you for your time,

Salvatore