Skip to contentSkip to conversation

st.ko: remove unnecessary normalize_buffer

PATCH Kai Mäkisara (Kolumbus) 2 participants 2 messages Dec 16, 2013 1 message not in the archive SCSI

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Kai Mäkisara (Kolumbus)

author
#1 lore raw
Quoted 11 lines · Bodo Stroesser

From: Bodo Stroesser <[email protected]>
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

Quoted 8 lines
Cc Kai Makisara <[email protected]>
Signed-off-by Bodo Stroesser <[email protected]>

---

drivers/scsi/st.c static int enlarge_buffer(struct st_buff · line 3730

Quoted 9 lines
drivers/scsi/st.c +0 -3
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw
Quoted 23 lines

-----Original Message-----
From: "Kai Mäkisara (Kolumbus)" [mailto:[email protected]]
Sent: Monday, December 16, 2013 7:45 PM
To: Strösser, Bodo
Cc: [email protected]
Subject: Re: [PATCH 2/3] st.ko: remove unnecessary normalize_buffer

Quoted 11 lines · Bodo Stroesser

From: Bodo Stroesser <[email protected]>
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.

Quoted 4 lines

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.

Quoted 8 lines

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

Quoted 16 lines

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

Thanks,
Kai

Quoted 11 lines
Cc Kai Makisara <[email protected]>
Signed-off-by Bodo Stroesser <[email protected]>

---

drivers/scsi/st.c +0 -0
@@ -3725,9 +3725,6 @@ static int enlarge_buffer(struct st_buff
 	if (new_size <= STbuffer->buffer_size)
 		return 1;
Quoted 6 lines

drivers/scsi/st.c static int enlarge_buffer(struct st_buff · line 3730

Quoted 6 lines
drivers/scsi/st.c +0 -3
-	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html