login
Header Space

 
 

Re: [PATCH 09/10] Change table chaining layout

Score:
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
To: Boaz Harrosh <bharrosh@...>
Cc: Jens Axboe <jens.axboe@...>, Alan Cox <alan@...>, Geert Uytterhoeven <geert@...>, Linux Kernel Development <linux-kernel@...>, <mingo@...>, Linux/m68k <linux-m68k@...>
Date: Tuesday, October 23, 2007 - 11:22 am

On Tue, 23 Oct 2007, Boaz Harrosh wrote:

Yes, that would be one clean situation.


Well, I'd personally actually prefer to *not* have the count be passed 
down explicitly, because it's just too error prone. So I'd much rather see 
the count implicit in the list: whether it's in an explicit header 
structure (that is the *only* thing passed down) or whether it's embedded 
in the list itself is not important.

Since the list itself has to have the "next pointer" for chaining, and 
thus already has "embedded information" in it, it actually does make sense 
in my opinion to just embed the end-of-list information too. And the end 
result right now is pretty simple, with "sg_next()" being really simple to 
use, and there being no way to screw things up by getting the count and 
the sg pointer out of sync.

My biggest complaint right now is that a lot of users of the sg *filling* 
functions were mindlessly converted, so we have code like

	cryptoloop.c:             sg_set_page(&sg_in, in_page);
	cryptoloop.c:             sg_in.offset = in_offs;
	cryptoloop.c:             sg_in.length = sz;

which is just really stupid, and we should have a function for that. But 
worse is code like this:

	ub.c:     sg_set_page(sg, virt_to_page(sc->top_sense));
	ub.c:     sg->offset = (unsigned long)sc->top_sense & (PAGE_SIZE-1);
	ub.c:     sg->length = UB_SENSE_SIZE;

which again was converted "line by line" and we actually *do* have a 
function to do the above three lines as

	sg_set_buf(sg, sc->top_sense, UB_SENSE_SIZE);

where that *single* line is just tons shorter but more importantly, more 
readable, than the mess that is a brute-force conversion.

So I think the SG stuff looks ok now, but I think we have a lot of "fix up 
the rough edges" to go!

(The above is not the only case. Just grep for "sg_set_page", and you'll 
see several examples of this kind of hard-to-read code. Basically, I don't 
think it's ever a good idea to initialize the SG entries one by one, and 
even when we have a hard page/offset/size thing, we should not set them 
one by one, and we should probably extend sg_set_page() to always take 
offset and length too, since setting one without the other two is never 
really sensible!)

		Linus
-
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
[PATCH 00/10] SG updates, Jens Axboe, (Mon Oct 22, 2:10 pm)
[PATCH][SG] fix typo in ps3rom.c, Arnd Bergmann, (Tue Oct 23, 10:48 am)
[PATCH 08/10] [SG] Update arch/ to use sg helpers, Jens Axboe, (Mon Oct 22, 2:11 pm)
Re: [PATCH 08/10] [SG] Update arch/ to use sg helpers, Benny Halevy, (Mon Oct 22, 5:10 pm)
[PATCH 07/10] [SG] Update swiotlb to use sg helpers, Jens Axboe, (Mon Oct 22, 2:11 pm)
[PATCH 06/10] [SG] Update net/ to use sg helpers, Jens Axboe, (Mon Oct 22, 2:11 pm)
Re: [PATCH 06/10] [SG] Update net/ to use sg helpers, Christian Borntraeger, (Tue Oct 23, 6:44 am)
Re: [PATCH 06/10] [SG] Update net/ to use sg helpers, Jens Axboe, (Tue Oct 23, 6:45 am)
[PATCH 05/10] [SG] Update fs/ to use sg helpers, Jens Axboe, (Mon Oct 22, 2:10 pm)
[PATCH 04/10] [SG] Update drivers to use sg helpers, Jens Axboe, (Mon Oct 22, 2:10 pm)
Re: [PATCH 04/10] [SG] Update drivers to use sg helpers, Heiko Carstens, (Tue Oct 23, 2:28 am)
Re: [PATCH 04/10] [SG] Update drivers to use sg helpers, Heiko Carstens, (Tue Oct 23, 3:16 am)
[PATCH 03/10] [SG] Update crypto/ to sg helpers, Jens Axboe, (Mon Oct 22, 2:10 pm)
[PATCH] fix ll_rw_blk.c build on s390, Heiko Carstens, (Tue Oct 23, 1:42 am)
[PATCH 10/10] Add CONFIG_DEBUG_SG sg validation, Jens Axboe, (Mon Oct 22, 2:11 pm)
[PATCH 09/10] Change table chaining layout, Jens Axboe, (Mon Oct 22, 2:11 pm)
Re: [PATCH 09/10] Change table chaining layout, Boaz Harrosh, (Tue Oct 23, 1:08 pm)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Tue Oct 23, 2:33 pm)
Re: [PATCH 09/10] Change table chaining layout, Andi Kleen, (Tue Oct 23, 3:56 pm)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Tue Oct 23, 4:20 pm)
Re: [PATCH 09/10] Change table chaining layout, Andi Kleen, (Tue Oct 23, 4:57 pm)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Tue Oct 23, 5:44 pm)
Re: [PATCH 09/10] Change table chaining layout, Geert Uytterhoeven, (Mon Oct 22, 3:39 pm)
Re: [PATCH 09/10] Change table chaining layout, Linus Torvalds, (Mon Oct 22, 3:49 pm)
Re: [PATCH 09/10] Change table chaining layout, Alan Cox, (Mon Oct 22, 4:16 pm)
Re: [PATCH 09/10] Change table chaining layout, Linus Torvalds, (Mon Oct 22, 4:44 pm)
Re: [PATCH 09/10] Change table chaining layout, Alan Cox, (Mon Oct 22, 5:43 pm)
Re: [PATCH 09/10] Change table chaining layout, Linus Torvalds, (Mon Oct 22, 5:47 pm)
Re: [PATCH 09/10] Change table chaining layout, Boaz Harrosh, (Tue Oct 23, 5:29 am)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Tue Oct 23, 5:41 am)
Re: [PATCH 09/10] Change table chaining layout, Ingo Molnar, (Tue Oct 23, 6:33 am)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Tue Oct 23, 6:56 am)
Re: [PATCH 09/10] Change table chaining layout, Ingo Molnar, (Tue Oct 23, 7:27 am)
Re: [PATCH 09/10] Change table chaining layout, Geert Uytterhoeven, (Tue Oct 23, 3:23 pm)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Wed Oct 24, 2:56 am)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Tue Oct 23, 5:46 pm)
Re: [PATCH 09/10] Change table chaining layout, Boaz Harrosh, (Tue Oct 23, 5:50 am)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Tue Oct 23, 5:55 am)
Re: [PATCH 09/10] Change table chaining layout, Boaz Harrosh, (Tue Oct 23, 6:23 am)
Re: [PATCH 09/10] Change table chaining layout, Linus Torvalds, (Tue Oct 23, 11:22 am)
Re: [PATCH 09/10] Change table chaining layout, Rusty Russell, (Thu Oct 25, 4:40 am)
Re: [PATCH 09/10] Change table chaining layout, Linus Torvalds, (Thu Oct 25, 11:40 am)
Re: [PATCH 09/10] Change table chaining layout, Paul Mackerras, (Fri Oct 26, 1:01 am)
Re: [PATCH 09/10] Change table chaining layout, Linus Torvalds, (Fri Oct 26, 10:52 am)
[RFC PATCH 1/2] sg_ring instead of scatterlist chaining, Rusty Russell, (Mon Nov 5, 2:11 am)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Fri Oct 26, 1:28 pm)
Re: [PATCH 09/10] Change table chaining layout, Benny Halevy, (Thu Oct 25, 12:03 pm)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Thu Oct 25, 5:11 am)
Re: [PATCH 09/10] Change table chaining layout, Rusty Russell, (Thu Oct 25, 7:54 am)
Re: [PATCH 09/10] Change table chaining layout, Rusty Russell, (Thu Oct 25, 8:03 pm)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Wed Oct 24, 4:05 am)
Re: [PATCH 09/10] Change table chaining layout, Geert Uytterhoeven, (Wed Oct 24, 5:03 am)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Wed Oct 24, 5:12 am)
Re: [PATCH 09/10] Change table chaining layout, Linus Torvalds, (Wed Oct 24, 11:16 am)
Re: [PATCH 09/10] Change table chaining layout, Olivier Galibert, (Wed Oct 24, 9:35 am)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Wed Oct 24, 9:38 am)
Re: [PATCH 09/10] Change table chaining layout, Olivier Galibert, (Wed Oct 24, 9:45 am)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Tue Oct 23, 6:29 am)
Re: [PATCH 09/10] Change table chaining layout, Geert Uytterhoeven, (Tue Oct 23, 3:18 am)
Re: [PATCH 09/10] Change table chaining layout, David Miller, (Mon Oct 22, 8:07 pm)
Re: [PATCH 09/10] Change table chaining layout, Jeff Garzik, (Mon Oct 22, 5:21 pm)
Re: [PATCH 09/10] Change table chaining layout, Matt Mackall, (Mon Oct 22, 5:47 pm)
Re: [PATCH 09/10] Change table chaining layout, Alan Cox, (Mon Oct 22, 6:52 pm)
Re: [PATCH 09/10] Change table chaining layout, Matt Mackall, (Mon Oct 22, 7:46 pm)
Re: [PATCH 09/10] Change table chaining layout, Jeff Garzik, (Mon Oct 22, 8:11 pm)
Re: [PATCH 09/10] Change table chaining layout, Benny Halevy, (Mon Oct 22, 5:16 pm)
Re: [PATCH 09/10] Change table chaining layout, Matt Mackall, (Mon Oct 22, 4:38 pm)
Re: [PATCH 09/10] Change table chaining layout, Jens Axboe, (Mon Oct 22, 3:52 pm)
powerpc: Fix fallout from sg_page() changes, Olof Johansson, (Tue Oct 23, 12:09 am)
Re: powerpc: Fix fallout from sg_page() changes, Jens Axboe, (Tue Oct 23, 3:13 am)
IB/ehca: Fix sg_page() fallout, Olof Johansson, (Tue Oct 23, 12:31 am)
Re: IB/ehca: Fix sg_page() fallout, Jens Axboe, (Tue Oct 23, 1:05 am)
Re: IB/ehca: Fix sg_page() fallout, Olof Johansson, (Tue Oct 23, 1:54 am)
Re: IB/ehca: Fix sg_page() fallout, Jens Axboe, (Tue Oct 23, 3:12 am)
speck-geostationary