[PATCH 2/3] st.ko: remove unnecessary normalize_buffer

[PATCH 2/3] st.ko: remove unnecessary normalize_buffer

[Bodo Stroesser]

From: Bodo Stroesser <bstroesser@ts.fujitsu.com>
Date: Mon, 2 Dec 2013 18:52:10 +0100
Subject: [PATCH 2/3] st.ko: remove unnecessary normalize_buffer

This patch removes an unnecessary call to normalize_buffer()
in enlarge_buffer()

In st_open() always a buffer of one page is allocated.
When the buffer needs to be enlarged later, it does not make
sense to free this page unconditionally.
 
Cc: Kai Makisara <kai.makisara@kolumbus.fi>
Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>

---

--- a/drivers/scsi/st.c	2013-12-02 18:52:10.000000000 +0100
+++ b/drivers/scsi/st.c	2013-12-02 18:52:10.000000000 +0100
@@ -3725,9 +3725,6 @@ static int enlarge_buffer(struct st_buff
 	if (new_size <= STbuffer->buffer_size)
 		return 1;
 
-	if (STbuffer->buffer_size <= PAGE_SIZE)
-		normalize_buffer(STbuffer);  /* Avoid extra segment */
-
 	max_segs = STbuffer->use_sg;
 
 	priority = GFP_KERNEL | __GFP_NOWARN;

Re: [PATCH 2/3] st.ko: remove unnecessary normalize_buffer

["Kai Mäkisara (Kolumbus)"]

On 2.12.2013, at 21.00, Bodo Stroesser <bstroesser@ts.fujitsu.com> wrote:

> From: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> Date: Mon, 2 Dec 2013 18:52:10 +0100
> Subject: [PATCH 2/3] st.ko: remove unnecessary normalize_buffer
> 
> This patch removes an unnecessary call to normalize_buffer()
> in enlarge_buffer()
> 
> In st_open() always a buffer of one page is allocated.
> When the buffer needs to be enlarged later, it does not make
> sense to free this page unconditionally.
> 

The original reason for this was to make the function to allocate the minimum number of segments for maximum efficiency. In some cases it was essential not to waste one segment because that would have made the allocation fail.

Now there is another reason to have this “optimization”. Reading the code you probably have noticed that nowadays all segments must have the same size. If the first segment is only one page, the first allocation may it may not be possible to allocate the buffer using single page segments. This leads to freeing the pages and trying again. This is not efficient.

So, I think this fragment of code should not be removed.

Thanks,
Kai

> Cc: Kai Makisara <kai.makisara@kolumbus.fi>
> Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> 
> ---
> 
> --- a/drivers/scsi/st.c	2013-12-02 18:52:10.000000000 +0100
> +++ b/drivers/scsi/st.c	2013-12-02 18:52:10.000000000 +0100
> @@ -3725,9 +3725,6 @@ static int enlarge_buffer(struct st_buff
> 	if (new_size <= STbuffer->buffer_size)
> 		return 1;
> 
> -	if (STbuffer->buffer_size <= PAGE_SIZE)
> -		normalize_buffer(STbuffer);  /* Avoid extra segment */
> -
> 	max_segs = STbuffer->use_sg;
> 
> 	priority = GFP_KERNEL | __GFP_NOWARN;

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/3] st.ko: remove unnecessary normalize_buffer

[Strösser, Bodo]

> -----Original Message-----
> From: "Kai Mäkisara (Kolumbus)" [mailto:kai.makisara@kolumbus.fi]
> Sent: Monday, December 16, 2013 7:45 PM
> To: Strösser, Bodo
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 2/3] st.ko: remove unnecessary normalize_buffer
> 
> 
> On 2.12.2013, at 21.00, Bodo Stroesser <bstroesser@ts.fujitsu.com>
> wrote:
> 
> > From: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> > Date: Mon, 2 Dec 2013 18:52:10 +0100
> > Subject: [PATCH 2/3] st.ko: remove unnecessary normalize_buffer
> >
> > This patch removes an unnecessary call to normalize_buffer()
> > in enlarge_buffer()
> >
> > In st_open() always a buffer of one page is allocated.
> > When the buffer needs to be enlarged later, it does not make
> > sense to free this page unconditionally.
> >
> 
> The original reason for this was to make the function to allocate the
> minimum number of segments for maximum efficiency. In some cases it was
> essential not to waste one segment because that would have made the
> allocation fail.

The method of enlarge_buffer now seems to have changed. It tries to
find out the minimum size of a segment that can be used with the given
sg list length. I assume, this method is used to be able to allocate
very big buffers, while in case of "normal" buffer still small chunks of
memory can be used, which under memory pressure are more likely to be
granted.

> 
> Now there is another reason to have this “optimization”. Reading the
> code you probably have noticed that nowadays all segments must have the
> same size. If the first segment is only one page, the first allocation
> may it may not be possible to allocate the buffer using single page
> segments. This leads to freeing the pages and trying again. This is not
> efficient.
> 

OTOH, if the size of the currently allocated segments is too small for
the buffer that needs to be allocated now, enlarge_buffer will free all
current segments and do a completely new allocation using bigger segments
anyway.

The lines, that the patch would remove, currently free one segment, if it
is the smallest possible and the only one, even if the same segment size
will be allocated again a few lines below. So I'd still call it a not
optimal handling of a special case, that not needs to be handled specially.

Thanks,
Bodo
 

> So, I think this fragment of code should not be removed.
> 
> Thanks,
> Kai
> 
> > Cc: Kai Makisara <kai.makisara@kolumbus.fi>
> > Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> >
> > ---
> >
> > --- a/drivers/scsi/st.c	2013-12-02 18:52:10.000000000 +0100
> > +++ b/drivers/scsi/st.c	2013-12-02 18:52:10.000000000 +0100
> > @@ -3725,9 +3725,6 @@ static int enlarge_buffer(struct st_buff
> > 	if (new_size <= STbuffer->buffer_size)
> > 		return 1;
> >
> > -	if (STbuffer->buffer_size <= PAGE_SIZE)
> > -		normalize_buffer(STbuffer);  /* Avoid extra segment */
> > -
> > 	max_segs = STbuffer->use_sg;
> >
> > 	priority = GFP_KERNEL | __GFP_NOWARN;

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html