Re: [PATCH 0/5] sg_ring for scsi
On Thursday 27 December 2007 13:09:27 FUJITA Tomonori wrote: > On Wed, 26 Dec 2007 11:27:40 +1100 > > Rusty Russell <[EMAIL PROTECTED]> wrote: > > There are many signs through the code that it needs a great deal of > > work: what is the purpose of sg_break? Why does the code check if > > kmap_atomic fails? > > sg_break is a workaround for the hardware restrictions, I think. Well, scb->breakup does that, can't see what scb->sg_break is for... May have to wade through git history to understand it. > > === > > Convert the ips SCSI driver to sg_ring. > > > > Slightly non-trivial conversion, will need testing. > > As I said, I don't think that converting SCSI drivers to sg_ring (with > lots of non-trivial work) provides any benefits. Surely, sg_ring > enables us to modify sg lists but SCSI drivers don't need the > feature. What SCSI drivers needs is just a efficient way to get the > next sg entry (they use 'sg++' in the past and sg_next now). Sure, iteration over a two-level structure is a little more tricky, but it's pretty trivial for most drivers. Indeed, I think that it's probably better to have an explicitly different interface for sg_ring, and keep the current interface for small sg arrays. That would provide a nicer changeover. I'll prototype something and see what it's like... Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Thursday 27 December 2007 13:09:27 FUJITA Tomonori wrote: On Wed, 26 Dec 2007 11:27:40 +1100 Rusty Russell [EMAIL PROTECTED] wrote: There are many signs through the code that it needs a great deal of work: what is the purpose of sg_break? Why does the code check if kmap_atomic fails? sg_break is a workaround for the hardware restrictions, I think. Well, scb-breakup does that, can't see what scb-sg_break is for... May have to wade through git history to understand it. === Convert the ips SCSI driver to sg_ring. Slightly non-trivial conversion, will need testing. As I said, I don't think that converting SCSI drivers to sg_ring (with lots of non-trivial work) provides any benefits. Surely, sg_ring enables us to modify sg lists but SCSI drivers don't need the feature. What SCSI drivers needs is just a efficient way to get the next sg entry (they use 'sg++' in the past and sg_next now). Sure, iteration over a two-level structure is a little more tricky, but it's pretty trivial for most drivers. Indeed, I think that it's probably better to have an explicitly different interface for sg_ring, and keep the current interface for small sg arrays. That would provide a nicer changeover. I'll prototype something and see what it's like... Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Wed, 26 Dec 2007 11:27:40 +1100
Rusty Russell <[EMAIL PROTECTED]> wrote:
> On Friday 21 December 2007 15:37:46 FUJITA Tomonori wrote:
> > Some scsi drivers like ips access to sglist in a tricky way.
>
> Indeed. I fail to see how this code works, in fact:
>
> drivers/scsi/ips.c:ips_queue() line 1101
>
> if (ips_is_passthru(SC)) {
>
> ips_copp_wait_item_t *scratch;
>
> /* A Reset IOCTL is only sent by the boot CD in extreme cases.
> */
> /* There can never be any system activity ( network or disk ),
> but check */
> /* anyway just as a good practice.
> */
> pt = (ips_passthru_t *) scsi_sglist(SC);
> if ((pt->CoppCP.cmd.reset.op_code == IPS_CMD_RESET_CHANNEL) &&
> (pt->CoppCP.cmd.reset.adapter_flag == 1)) {
>
> Casting the scsi_sglist(SC) (which ips_is_passthrough treated as a
> scatterlist) to an ips_passthrough_t seems completely bogus. It looks like
> it wants to access the region mapped by the scatterlist.
Yeah, it seems to be broken.
> There are many signs through the code that it needs a great deal of
> work: what is the purpose of sg_break? Why does the code check if kmap_atomic
> fails?
sg_break is a workaround for the hardware restrictions, I think.
> ===
> Convert the ips SCSI driver to sg_ring.
>
> Slightly non-trivial conversion, will need testing.
As I said, I don't think that converting SCSI drivers to sg_ring (with
lots of non-trivial work) provides any benefits. Surely, sg_ring
enables us to modify sg lists but SCSI drivers don't need the
feature. What SCSI drivers needs is just a efficient way to get the
next sg entry (they use 'sg++' in the past and sg_next now).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Wed, 26 Dec 2007 11:27:40 +1100
Rusty Russell [EMAIL PROTECTED] wrote:
On Friday 21 December 2007 15:37:46 FUJITA Tomonori wrote:
Some scsi drivers like ips access to sglist in a tricky way.
Indeed. I fail to see how this code works, in fact:
drivers/scsi/ips.c:ips_queue() line 1101
if (ips_is_passthru(SC)) {
ips_copp_wait_item_t *scratch;
/* A Reset IOCTL is only sent by the boot CD in extreme cases.
*/
/* There can never be any system activity ( network or disk ),
but check */
/* anyway just as a good practice.
*/
pt = (ips_passthru_t *) scsi_sglist(SC);
if ((pt-CoppCP.cmd.reset.op_code == IPS_CMD_RESET_CHANNEL)
(pt-CoppCP.cmd.reset.adapter_flag == 1)) {
Casting the scsi_sglist(SC) (which ips_is_passthrough treated as a
scatterlist) to an ips_passthrough_t seems completely bogus. It looks like
it wants to access the region mapped by the scatterlist.
Yeah, it seems to be broken.
There are many signs through the code that it needs a great deal of
work: what is the purpose of sg_break? Why does the code check if kmap_atomic
fails?
sg_break is a workaround for the hardware restrictions, I think.
===
Convert the ips SCSI driver to sg_ring.
Slightly non-trivial conversion, will need testing.
As I said, I don't think that converting SCSI drivers to sg_ring (with
lots of non-trivial work) provides any benefits. Surely, sg_ring
enables us to modify sg lists but SCSI drivers don't need the
feature. What SCSI drivers needs is just a efficient way to get the
next sg entry (they use 'sg++' in the past and sg_next now).
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Friday 21 December 2007 15:37:46 FUJITA Tomonori wrote:
> Some scsi drivers like ips access to sglist in a tricky way.
Indeed. I fail to see how this code works, in fact:
drivers/scsi/ips.c:ips_queue() line 1101
if (ips_is_passthru(SC)) {
ips_copp_wait_item_t *scratch;
/* A Reset IOCTL is only sent by the boot CD in extreme cases.
*/
/* There can never be any system activity ( network or disk ),
but check */
/* anyway just as a good practice.
*/
pt = (ips_passthru_t *) scsi_sglist(SC);
if ((pt->CoppCP.cmd.reset.op_code == IPS_CMD_RESET_CHANNEL) &&
(pt->CoppCP.cmd.reset.adapter_flag == 1)) {
Casting the scsi_sglist(SC) (which ips_is_passthrough treated as a
scatterlist) to an ips_passthrough_t seems completely bogus. It looks like
it wants to access the region mapped by the scatterlist.
There are many signs through the code that it needs a great deal of work:
what is the purpose of sg_break? Why does the code check if kmap_atomic
fails?
===
Convert the ips SCSI driver to sg_ring.
Slightly non-trivial conversion, will need testing.
The first issue is the standard one with "scsi_for_each_sg"
conversion: scsi_for_each_sg will always start i at 0 and increment it
each time around; "sg_ring_for_each" will start i at 0 but it will
return to zero for each new sg_ring entry; you need a separate counter
if you really want a simple increment.
Various flaws in the driver have been maintained.
Signed-off-by: Rusty Russell <[EMAIL PROTECTED]>
diff -r 63176a8a6ce3 drivers/scsi/ips.c
--- a/drivers/scsi/ips.cMon Dec 24 17:40:08 2007 +1100
+++ b/drivers/scsi/ips.cWed Dec 26 11:20:52 2007 +1100
@@ -1105,7 +1105,7 @@ static int ips_queue(struct scsi_cmnd *S
/* A Reset IOCTL is only sent by the boot CD in extreme cases.
*/
/* There can never be any system activity ( network or disk ),
but check */
/* anyway just as a good practice.
*/
- pt = (ips_passthru_t *) scsi_sglist(SC);
+ pt = (ips_passthru_t *) SC->sg;
if ((pt->CoppCP.cmd.reset.op_code == IPS_CMD_RESET_CHANNEL) &&
(pt->CoppCP.cmd.reset.adapter_flag == 1)) {
if (ha->scb_activelist.count != 0) {
@@ -1508,8 +1508,8 @@ static int ips_is_passthru(struct scsi_c
if ((SC->cmnd[0] == IPS_IOCTL_COMMAND) &&
(SC->device->channel == 0) &&
(SC->device->id == IPS_ADAPTER_ID) &&
- (SC->device->lun == 0) && scsi_sglist(SC)) {
-struct scatterlist *sg = scsi_sglist(SC);
+ (SC->device->lun == 0) && SC->sg) {
+struct scatterlist *sg = >sg->sg[0];
char *buffer;
/* kmap_atomic() ensures addressability of the user buffer.*/
@@ -1575,12 +1575,12 @@ ips_make_passthru(ips_ha_t *ha, struct s
ips_passthru_t *pt;
int length = 0;
int i, ret;
-struct scatterlist *sg = scsi_sglist(SC);
+struct sg_ring *sg;
METHOD_TRACE("ips_make_passthru", 1);
-scsi_for_each_sg(SC, sg, scsi_sg_count(SC), i)
-length += sg[i].length;
+ sg_ring_for_each(SC->sg, sg, i)
+length += sg->sg[i].length;
if (length < sizeof (ips_passthru_t)) {
/* wrong size */
@@ -2005,7 +2005,7 @@ ips_cleanup_passthru(ips_ha_t * ha, ips_
METHOD_TRACE("ips_cleanup_passthru", 1);
- if ((!scb) || (!scb->scsi_cmd) || (!scsi_sglist(scb->scsi_cmd))) {
+ if ((!scb) || (!scb->scsi_cmd) || (!scb->scsi_cmd->sg)) {
DEBUG_VAR(1, "(%s%d) couldn't cleanup after passthru",
ips_name, ha->host_num);
@@ -2758,16 +2758,18 @@ ips_next(ips_ha_t * ha, int intr)
scb->sg_count = scsi_dma_map(SC);
BUG_ON(scb->sg_count < 0);
if (scb->sg_count) {
- struct scatterlist *sg;
- int i;
+ struct sg_ring *sg;
+ int i, idx;
scb->flags |= IPS_SCB_MAP_SG;
-scsi_for_each_sg(SC, sg, scb->sg_count, i) {
+ idx = 0;
+ sg_ring_for_each(SC->sg, sg, i) {
if (ips_fill_scb_sg_single
- (ha, sg_dma_address(sg), scb, i,
-sg_dma_len(sg)) < 0)
- break;
+ (ha, sg_dma_address(>sg[i]), scb, idx,
+sg_dma_len(>sg[i])) < 0)
+ break;
+ idx++;
}
Re: [PATCH 0/5] sg_ring for scsi
On Friday 21 December 2007 15:37:46 FUJITA Tomonori wrote:
Some scsi drivers like ips access to sglist in a tricky way.
Indeed. I fail to see how this code works, in fact:
drivers/scsi/ips.c:ips_queue() line 1101
if (ips_is_passthru(SC)) {
ips_copp_wait_item_t *scratch;
/* A Reset IOCTL is only sent by the boot CD in extreme cases.
*/
/* There can never be any system activity ( network or disk ),
but check */
/* anyway just as a good practice.
*/
pt = (ips_passthru_t *) scsi_sglist(SC);
if ((pt-CoppCP.cmd.reset.op_code == IPS_CMD_RESET_CHANNEL)
(pt-CoppCP.cmd.reset.adapter_flag == 1)) {
Casting the scsi_sglist(SC) (which ips_is_passthrough treated as a
scatterlist) to an ips_passthrough_t seems completely bogus. It looks like
it wants to access the region mapped by the scatterlist.
There are many signs through the code that it needs a great deal of work:
what is the purpose of sg_break? Why does the code check if kmap_atomic
fails?
===
Convert the ips SCSI driver to sg_ring.
Slightly non-trivial conversion, will need testing.
The first issue is the standard one with scsi_for_each_sg
conversion: scsi_for_each_sg will always start i at 0 and increment it
each time around; sg_ring_for_each will start i at 0 but it will
return to zero for each new sg_ring entry; you need a separate counter
if you really want a simple increment.
Various flaws in the driver have been maintained.
Signed-off-by: Rusty Russell [EMAIL PROTECTED]
diff -r 63176a8a6ce3 drivers/scsi/ips.c
--- a/drivers/scsi/ips.cMon Dec 24 17:40:08 2007 +1100
+++ b/drivers/scsi/ips.cWed Dec 26 11:20:52 2007 +1100
@@ -1105,7 +1105,7 @@ static int ips_queue(struct scsi_cmnd *S
/* A Reset IOCTL is only sent by the boot CD in extreme cases.
*/
/* There can never be any system activity ( network or disk ),
but check */
/* anyway just as a good practice.
*/
- pt = (ips_passthru_t *) scsi_sglist(SC);
+ pt = (ips_passthru_t *) SC-sg;
if ((pt-CoppCP.cmd.reset.op_code == IPS_CMD_RESET_CHANNEL)
(pt-CoppCP.cmd.reset.adapter_flag == 1)) {
if (ha-scb_activelist.count != 0) {
@@ -1508,8 +1508,8 @@ static int ips_is_passthru(struct scsi_c
if ((SC-cmnd[0] == IPS_IOCTL_COMMAND)
(SC-device-channel == 0)
(SC-device-id == IPS_ADAPTER_ID)
- (SC-device-lun == 0) scsi_sglist(SC)) {
-struct scatterlist *sg = scsi_sglist(SC);
+ (SC-device-lun == 0) SC-sg) {
+struct scatterlist *sg = SC-sg-sg[0];
char *buffer;
/* kmap_atomic() ensures addressability of the user buffer.*/
@@ -1575,12 +1575,12 @@ ips_make_passthru(ips_ha_t *ha, struct s
ips_passthru_t *pt;
int length = 0;
int i, ret;
-struct scatterlist *sg = scsi_sglist(SC);
+struct sg_ring *sg;
METHOD_TRACE(ips_make_passthru, 1);
-scsi_for_each_sg(SC, sg, scsi_sg_count(SC), i)
-length += sg[i].length;
+ sg_ring_for_each(SC-sg, sg, i)
+length += sg-sg[i].length;
if (length sizeof (ips_passthru_t)) {
/* wrong size */
@@ -2005,7 +2005,7 @@ ips_cleanup_passthru(ips_ha_t * ha, ips_
METHOD_TRACE(ips_cleanup_passthru, 1);
- if ((!scb) || (!scb-scsi_cmd) || (!scsi_sglist(scb-scsi_cmd))) {
+ if ((!scb) || (!scb-scsi_cmd) || (!scb-scsi_cmd-sg)) {
DEBUG_VAR(1, (%s%d) couldn't cleanup after passthru,
ips_name, ha-host_num);
@@ -2758,16 +2758,18 @@ ips_next(ips_ha_t * ha, int intr)
scb-sg_count = scsi_dma_map(SC);
BUG_ON(scb-sg_count 0);
if (scb-sg_count) {
- struct scatterlist *sg;
- int i;
+ struct sg_ring *sg;
+ int i, idx;
scb-flags |= IPS_SCB_MAP_SG;
-scsi_for_each_sg(SC, sg, scb-sg_count, i) {
+ idx = 0;
+ sg_ring_for_each(SC-sg, sg, i) {
if (ips_fill_scb_sg_single
- (ha, sg_dma_address(sg), scb, i,
-sg_dma_len(sg)) 0)
- break;
+ (ha, sg_dma_address(sg-sg[i]), scb, idx,
+sg_dma_len(sg-sg[i])) 0)
+ break;
+ idx++;
}
scb-dcdb.transfer_length = scb-data_len;
} else {
Re: [PATCH 0/5] sg_ring for scsi
Rusty Russell <[EMAIL PROTECTED]> wrote:
>
> 2) It allows others to manipulate sg chains, which couldn't be done before
> (eg. the ATA code which wants to append a padding element).
Yes chaining at the end is very useful for the crypto layer as
well.
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
Rusty Russell [EMAIL PROTECTED] wrote:
2) It allows others to manipulate sg chains, which couldn't be done before
(eg. the ATA code which wants to append a padding element).
Yes chaining at the end is very useful for the crypto layer as
well.
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Fri, 21 Dec 2007 14:26:47 +1100 Rusty Russell <[EMAIL PROTECTED]> wrote: > On Friday 21 December 2007 13:28:34 FUJITA Tomonori wrote: > > I'm not sure about chaining the headers (as your sg_ring and > > scsi_sgtable do) would simplify LLDs. Have you looked at ips or > > qla1280? > > Not yet, am working my way through the drivers, but I don't expect it will be > a simplification to the normal SCSI LLDs. Most of them are mere consumers of > sgs... Some scsi drivers like ips access to sglist in a tricky way. I feel that they don't work with the sg_ring interface well. So if you convert scsi_lib.c to use sg_ring, please see how it works with the tricky drivers before that. > I'm not a SCSI person: I'm patching SCSI because I have to to get my > own sg-using code clean :) I'm SCSI-biased. If you don't convert scsi to use sg_ring, I don't complain. :) Though it would be better to have only one mechanism to handle large sglist in kernel. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Friday 21 December 2007 13:28:34 FUJITA Tomonori wrote: > I'm not sure about chaining the headers (as your sg_ring and > scsi_sgtable do) would simplify LLDs. Have you looked at ips or > qla1280? Not yet, am working my way through the drivers, but I don't expect it will be a simplification to the normal SCSI LLDs. Most of them are mere consumers of sgs... I'm not a SCSI person: I'm patching SCSI because I have to to get my own sg-using code clean :) Hope that clarifies, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Fri, 21 Dec 2007 10:13:38 +1100 Rusty Russell <[EMAIL PROTECTED]> wrote: > On Thursday 20 December 2007 18:58:07 David Miller wrote: > > From: Rusty Russell <[EMAIL PROTECTED]> > > Date: Thu, 20 Dec 2007 18:53:48 +1100 > > > > > Manipulating the magic chains is horrible; it looks simple to the > > > places which simply want to iterate through it, but it's awful for > > > code which wants to create them. > > > > I'm not saying complexity is inherent in this stuff, but > > assuming that it is the complexity should live as far away > > from the minions (the iterators in this case). Therefore, > > the creators is the right spot for the hard stuff. > > In this case, the main benefit of the sg chaining was that the conversion of > most scsi drivers was easy (basically sg++ -> sg = sg_next(sg)). The > conversion to sg_ring is more complex, but the end result is not > significantly more complex. > > However, the cost to code which manipulates sg chains was significant: I > tried > using them in virtio and it was too ugly to live (so that doesn't support sg > chaining). If this was the best we could do, that'd be fine. > > But, as demonstrated, there are real benefits of having an explicit header: I'm not sure about chaining the headers (as your sg_ring and scsi_sgtable do) would simplify LLDs. Have you looked at ips or qla1280? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
From: Rusty Russell <[EMAIL PROTECTED]> Date: Fri, 21 Dec 2007 10:13:38 +1100 > But, as demonstrated, there are real benefits of having an explicit header: > > 1) It removes the chain-end/explicit count ambiguity (see > http://lkml.org/lkml/2007/10/25/209 & thread) > 2) It allows others to manipulate sg chains, which couldn't be done before >(eg. the ATA code which wants to append a padding element). > 3) I can now hand you an sg ring for you to fill: sg chains can't do that. > > In short, sg_ring is generally useful primitive. sg chains are a clever hack > for scsi_lib to create, and everyone else to read. I do not refute any of this :-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Thursday 20 December 2007 18:58:07 David Miller wrote: > From: Rusty Russell <[EMAIL PROTECTED]> > Date: Thu, 20 Dec 2007 18:53:48 +1100 > > > Manipulating the magic chains is horrible; it looks simple to the > > places which simply want to iterate through it, but it's awful for > > code which wants to create them. > > I'm not saying complexity is inherent in this stuff, but > assuming that it is the complexity should live as far away > from the minions (the iterators in this case). Therefore, > the creators is the right spot for the hard stuff. In this case, the main benefit of the sg chaining was that the conversion of most scsi drivers was easy (basically sg++ -> sg = sg_next(sg)). The conversion to sg_ring is more complex, but the end result is not significantly more complex. However, the cost to code which manipulates sg chains was significant: I tried using them in virtio and it was too ugly to live (so that doesn't support sg chaining). If this was the best we could do, that'd be fine. But, as demonstrated, there are real benefits of having an explicit header: 1) It removes the chain-end/explicit count ambiguity (see http://lkml.org/lkml/2007/10/25/209 & thread) 2) It allows others to manipulate sg chains, which couldn't be done before (eg. the ATA code which wants to append a padding element). 3) I can now hand you an sg ring for you to fill: sg chains can't do that. In short, sg_ring is generally useful primitive. sg chains are a clever hack for scsi_lib to create, and everyone else to read. Hope that clarifies, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Thu, Dec 20 2007 at 9:58 +0200, Jens Axboe <[EMAIL PROTECTED]> wrote: > On Thu, Dec 20 2007, Rusty Russell wrote: >> On Thursday 20 December 2007 18:07:41 FUJITA Tomonori wrote: >>> On Thu, 20 Dec 2007 16:45:18 +1100 >>> >>> Rusty Russell <[EMAIL PROTECTED]> wrote: OK, some fixes since last time, as I wade through more SCSI drivers. Some drivers use "use_sg" as a flag to know whether the request_buffer is a scatterlist: I don't need the counter, but I still need the flag, so I fixed that in a more intuitive way (an explicit ->sg pointer in the cmd). >>> use_sg and the request_buffer will be removed shortly. >>> >>> http://marc.info/?l=linux-scsi=119754650614813=2 >> Thanks! Is there a git tree somewhere with these changes? >> >>> I think that we tried the similar idea before, scsi_sgtable, but we >>> seem to settle in the current simple approach. >> Yes, a scsi-specific solution is a bad idea: other people use sg. >> Manipulating the magic chains is horrible; it looks simple to the places >> which simply want to iterate through it, but it's awful for code which wants >> to create them. > > The current code looks like that to minimize impact on 2.6.24, see this > branch: > > http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=sg > > for how it folds into lib/sg.c and the magic disappears from SCSI. > Rusty, nobody claimed the sg code in 2.6.24 is perfect. I like to get > things incrementally there. > Dear Jens. Is this code scheduled for 2.6.25? I cannot find it in mm tree. Because above code conflicts with the scsi_data_buffer patch, that is in mm tree and I hope will get accepted into 2.6.25. Now the concepts are not at all conflicting, only that they both bang in the same places. (And by the way it does not apply on scsi-misc either. And it did not compile in your tree, a missing bit in ide-scsi.c) I have rebase and fixed your code on top of scsi_data_buffer patchset. Please review. (Patchset posted as reply to this mail) They are totaly untested, based on -mm tree. We should decide the order of these patches and rebase accordingly. AND ... Please send, to-be-included-in-next-kernel patches to Morton. This way we can account for them. Also I do not see Ack-by: of the scsi maintainer in the scsi bits of your patches. Is it not a costume to Ack-on bits that belong to other maintainers, even for maintainers? Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Wed, Dec 19 2007, David Miller wrote: > From: Rusty Russell <[EMAIL PROTECTED]> > Date: Thu, 20 Dec 2007 18:53:48 +1100 > > > Manipulating the magic chains is horrible; it looks simple to the > > places which simply want to iterate through it, but it's awful for > > code which wants to create them. > > I'm not saying complexity is inherent in this stuff, but > assuming that it is the complexity should live as far away > from the minions (the iterators in this case). Therefore, > the creators is the right spot for the hard stuff. Agree, and the missing bit is just moving the creators out of the driver parts and into a core helper. See the previous post on the 'sg' branch for 2.6.25. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Thu, Dec 20 2007, Rusty Russell wrote: > On Thursday 20 December 2007 18:07:41 FUJITA Tomonori wrote: > > On Thu, 20 Dec 2007 16:45:18 +1100 > > > > Rusty Russell <[EMAIL PROTECTED]> wrote: > > > OK, some fixes since last time, as I wade through more SCSI drivers. > > > Some drivers use "use_sg" as a flag to know whether the request_buffer is > > > a scatterlist: I don't need the counter, but I still need the flag, so I > > > fixed that in a more intuitive way (an explicit ->sg pointer in the cmd). > > > > use_sg and the request_buffer will be removed shortly. > > > > http://marc.info/?l=linux-scsi=119754650614813=2 > > Thanks! Is there a git tree somewhere with these changes? > > > I think that we tried the similar idea before, scsi_sgtable, but we > > seem to settle in the current simple approach. > > Yes, a scsi-specific solution is a bad idea: other people use sg. > Manipulating the magic chains is horrible; it looks simple to the places > which simply want to iterate through it, but it's awful for code which wants > to create them. The current code looks like that to minimize impact on 2.6.24, see this branch: http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=sg for how it folds into lib/sg.c and the magic disappears from SCSI. Rusty, nobody claimed the sg code in 2.6.24 is perfect. I like to get things incrementally there. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Thu, Dec 20 2007, Rusty Russell wrote: On Thursday 20 December 2007 18:07:41 FUJITA Tomonori wrote: On Thu, 20 Dec 2007 16:45:18 +1100 Rusty Russell [EMAIL PROTECTED] wrote: OK, some fixes since last time, as I wade through more SCSI drivers. Some drivers use use_sg as a flag to know whether the request_buffer is a scatterlist: I don't need the counter, but I still need the flag, so I fixed that in a more intuitive way (an explicit -sg pointer in the cmd). use_sg and the request_buffer will be removed shortly. http://marc.info/?l=linux-scsim=119754650614813w=2 Thanks! Is there a git tree somewhere with these changes? I think that we tried the similar idea before, scsi_sgtable, but we seem to settle in the current simple approach. Yes, a scsi-specific solution is a bad idea: other people use sg. Manipulating the magic chains is horrible; it looks simple to the places which simply want to iterate through it, but it's awful for code which wants to create them. The current code looks like that to minimize impact on 2.6.24, see this branch: http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=sg for how it folds into lib/sg.c and the magic disappears from SCSI. Rusty, nobody claimed the sg code in 2.6.24 is perfect. I like to get things incrementally there. -- Jens Axboe -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Wed, Dec 19 2007, David Miller wrote: From: Rusty Russell [EMAIL PROTECTED] Date: Thu, 20 Dec 2007 18:53:48 +1100 Manipulating the magic chains is horrible; it looks simple to the places which simply want to iterate through it, but it's awful for code which wants to create them. I'm not saying complexity is inherent in this stuff, but assuming that it is the complexity should live as far away from the minions (the iterators in this case). Therefore, the creators is the right spot for the hard stuff. Agree, and the missing bit is just moving the creators out of the driver parts and into a core helper. See the previous post on the 'sg' branch for 2.6.25. -- Jens Axboe -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Thu, Dec 20 2007 at 9:58 +0200, Jens Axboe [EMAIL PROTECTED] wrote: On Thu, Dec 20 2007, Rusty Russell wrote: On Thursday 20 December 2007 18:07:41 FUJITA Tomonori wrote: On Thu, 20 Dec 2007 16:45:18 +1100 Rusty Russell [EMAIL PROTECTED] wrote: OK, some fixes since last time, as I wade through more SCSI drivers. Some drivers use use_sg as a flag to know whether the request_buffer is a scatterlist: I don't need the counter, but I still need the flag, so I fixed that in a more intuitive way (an explicit -sg pointer in the cmd). use_sg and the request_buffer will be removed shortly. http://marc.info/?l=linux-scsim=119754650614813w=2 Thanks! Is there a git tree somewhere with these changes? I think that we tried the similar idea before, scsi_sgtable, but we seem to settle in the current simple approach. Yes, a scsi-specific solution is a bad idea: other people use sg. Manipulating the magic chains is horrible; it looks simple to the places which simply want to iterate through it, but it's awful for code which wants to create them. The current code looks like that to minimize impact on 2.6.24, see this branch: http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=sg for how it folds into lib/sg.c and the magic disappears from SCSI. Rusty, nobody claimed the sg code in 2.6.24 is perfect. I like to get things incrementally there. Dear Jens. Is this code scheduled for 2.6.25? I cannot find it in mm tree. Because above code conflicts with the scsi_data_buffer patch, that is in mm tree and I hope will get accepted into 2.6.25. Now the concepts are not at all conflicting, only that they both bang in the same places. (And by the way it does not apply on scsi-misc either. And it did not compile in your tree, a missing bit in ide-scsi.c) I have rebase and fixed your code on top of scsi_data_buffer patchset. Please review. (Patchset posted as reply to this mail) They are totaly untested, based on -mm tree. We should decide the order of these patches and rebase accordingly. AND ... Please send, to-be-included-in-next-kernel patches to Morton. This way we can account for them. Also I do not see Ack-by: of the scsi maintainer in the scsi bits of your patches. Is it not a costume to Ack-on bits that belong to other maintainers, even for maintainers? Boaz -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Thursday 20 December 2007 18:58:07 David Miller wrote: From: Rusty Russell [EMAIL PROTECTED] Date: Thu, 20 Dec 2007 18:53:48 +1100 Manipulating the magic chains is horrible; it looks simple to the places which simply want to iterate through it, but it's awful for code which wants to create them. I'm not saying complexity is inherent in this stuff, but assuming that it is the complexity should live as far away from the minions (the iterators in this case). Therefore, the creators is the right spot for the hard stuff. In this case, the main benefit of the sg chaining was that the conversion of most scsi drivers was easy (basically sg++ - sg = sg_next(sg)). The conversion to sg_ring is more complex, but the end result is not significantly more complex. However, the cost to code which manipulates sg chains was significant: I tried using them in virtio and it was too ugly to live (so that doesn't support sg chaining). If this was the best we could do, that'd be fine. But, as demonstrated, there are real benefits of having an explicit header: 1) It removes the chain-end/explicit count ambiguity (see http://lkml.org/lkml/2007/10/25/209 thread) 2) It allows others to manipulate sg chains, which couldn't be done before (eg. the ATA code which wants to append a padding element). 3) I can now hand you an sg ring for you to fill: sg chains can't do that. In short, sg_ring is generally useful primitive. sg chains are a clever hack for scsi_lib to create, and everyone else to read. Hope that clarifies, Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
From: Rusty Russell [EMAIL PROTECTED] Date: Fri, 21 Dec 2007 10:13:38 +1100 But, as demonstrated, there are real benefits of having an explicit header: 1) It removes the chain-end/explicit count ambiguity (see http://lkml.org/lkml/2007/10/25/209 thread) 2) It allows others to manipulate sg chains, which couldn't be done before (eg. the ATA code which wants to append a padding element). 3) I can now hand you an sg ring for you to fill: sg chains can't do that. In short, sg_ring is generally useful primitive. sg chains are a clever hack for scsi_lib to create, and everyone else to read. I do not refute any of this :-) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Fri, 21 Dec 2007 10:13:38 +1100 Rusty Russell [EMAIL PROTECTED] wrote: On Thursday 20 December 2007 18:58:07 David Miller wrote: From: Rusty Russell [EMAIL PROTECTED] Date: Thu, 20 Dec 2007 18:53:48 +1100 Manipulating the magic chains is horrible; it looks simple to the places which simply want to iterate through it, but it's awful for code which wants to create them. I'm not saying complexity is inherent in this stuff, but assuming that it is the complexity should live as far away from the minions (the iterators in this case). Therefore, the creators is the right spot for the hard stuff. In this case, the main benefit of the sg chaining was that the conversion of most scsi drivers was easy (basically sg++ - sg = sg_next(sg)). The conversion to sg_ring is more complex, but the end result is not significantly more complex. However, the cost to code which manipulates sg chains was significant: I tried using them in virtio and it was too ugly to live (so that doesn't support sg chaining). If this was the best we could do, that'd be fine. But, as demonstrated, there are real benefits of having an explicit header: I'm not sure about chaining the headers (as your sg_ring and scsi_sgtable do) would simplify LLDs. Have you looked at ips or qla1280? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Friday 21 December 2007 13:28:34 FUJITA Tomonori wrote: I'm not sure about chaining the headers (as your sg_ring and scsi_sgtable do) would simplify LLDs. Have you looked at ips or qla1280? Not yet, am working my way through the drivers, but I don't expect it will be a simplification to the normal SCSI LLDs. Most of them are mere consumers of sgs... I'm not a SCSI person: I'm patching SCSI because I have to to get my own sg-using code clean :) Hope that clarifies, Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Fri, 21 Dec 2007 14:26:47 +1100 Rusty Russell [EMAIL PROTECTED] wrote: On Friday 21 December 2007 13:28:34 FUJITA Tomonori wrote: I'm not sure about chaining the headers (as your sg_ring and scsi_sgtable do) would simplify LLDs. Have you looked at ips or qla1280? Not yet, am working my way through the drivers, but I don't expect it will be a simplification to the normal SCSI LLDs. Most of them are mere consumers of sgs... Some scsi drivers like ips access to sglist in a tricky way. I feel that they don't work with the sg_ring interface well. So if you convert scsi_lib.c to use sg_ring, please see how it works with the tricky drivers before that. I'm not a SCSI person: I'm patching SCSI because I have to to get my own sg-using code clean :) I'm SCSI-biased. If you don't convert scsi to use sg_ring, I don't complain. :) Though it would be better to have only one mechanism to handle large sglist in kernel. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
From: Rusty Russell <[EMAIL PROTECTED]> Date: Thu, 20 Dec 2007 18:53:48 +1100 > Manipulating the magic chains is horrible; it looks simple to the > places which simply want to iterate through it, but it's awful for > code which wants to create them. I'm not saying complexity is inherent in this stuff, but assuming that it is the complexity should live as far away from the minions (the iterators in this case). Therefore, the creators is the right spot for the hard stuff. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Thursday 20 December 2007 18:07:41 FUJITA Tomonori wrote: > On Thu, 20 Dec 2007 16:45:18 +1100 > > Rusty Russell <[EMAIL PROTECTED]> wrote: > > OK, some fixes since last time, as I wade through more SCSI drivers. > > Some drivers use "use_sg" as a flag to know whether the request_buffer is > > a scatterlist: I don't need the counter, but I still need the flag, so I > > fixed that in a more intuitive way (an explicit ->sg pointer in the cmd). > > use_sg and the request_buffer will be removed shortly. > > http://marc.info/?l=linux-scsi=119754650614813=2 Thanks! Is there a git tree somewhere with these changes? > I think that we tried the similar idea before, scsi_sgtable, but we > seem to settle in the current simple approach. Yes, a scsi-specific solution is a bad idea: other people use sg. Manipulating the magic chains is horrible; it looks simple to the places which simply want to iterate through it, but it's awful for code which wants to create them. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Thu, 20 Dec 2007 16:45:18 +1100 Rusty Russell <[EMAIL PROTECTED]> wrote: > OK, some fixes since last time, as I wade through more SCSI drivers. Some > drivers use "use_sg" as a flag to know whether the request_buffer is a > scatterlist: I don't need the counter, but I still need the flag, so I fixed > that in a more intuitive way (an explicit ->sg pointer in the cmd). use_sg and the request_buffer will be removed shortly. http://marc.info/?l=linux-scsi=119754650614813=2 I think that we tried the similar idea before, scsi_sgtable, but we seem to settle in the current simple approach. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/5] sg_ring for scsi
OK, some fixes since last time, as I wade through more SCSI drivers. Some drivers use "use_sg" as a flag to know whether the request_buffer is a scatterlist: I don't need the counter, but I still need the flag, so I fixed that in a more intuitive way (an explicit ->sg pointer in the cmd). Also, I've updated and tested scsi_debug, after Douglas's excellent suggestion. (I just found out about struct scsi_pointer, so I'm off to update that now, too). Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/5] sg_ring for scsi
OK, some fixes since last time, as I wade through more SCSI drivers. Some drivers use use_sg as a flag to know whether the request_buffer is a scatterlist: I don't need the counter, but I still need the flag, so I fixed that in a more intuitive way (an explicit -sg pointer in the cmd). Also, I've updated and tested scsi_debug, after Douglas's excellent suggestion. (I just found out about struct scsi_pointer, so I'm off to update that now, too). Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Thu, 20 Dec 2007 16:45:18 +1100 Rusty Russell [EMAIL PROTECTED] wrote: OK, some fixes since last time, as I wade through more SCSI drivers. Some drivers use use_sg as a flag to know whether the request_buffer is a scatterlist: I don't need the counter, but I still need the flag, so I fixed that in a more intuitive way (an explicit -sg pointer in the cmd). use_sg and the request_buffer will be removed shortly. http://marc.info/?l=linux-scsim=119754650614813w=2 I think that we tried the similar idea before, scsi_sgtable, but we seem to settle in the current simple approach. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
On Thursday 20 December 2007 18:07:41 FUJITA Tomonori wrote: On Thu, 20 Dec 2007 16:45:18 +1100 Rusty Russell [EMAIL PROTECTED] wrote: OK, some fixes since last time, as I wade through more SCSI drivers. Some drivers use use_sg as a flag to know whether the request_buffer is a scatterlist: I don't need the counter, but I still need the flag, so I fixed that in a more intuitive way (an explicit -sg pointer in the cmd). use_sg and the request_buffer will be removed shortly. http://marc.info/?l=linux-scsim=119754650614813w=2 Thanks! Is there a git tree somewhere with these changes? I think that we tried the similar idea before, scsi_sgtable, but we seem to settle in the current simple approach. Yes, a scsi-specific solution is a bad idea: other people use sg. Manipulating the magic chains is horrible; it looks simple to the places which simply want to iterate through it, but it's awful for code which wants to create them. Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] sg_ring for scsi
From: Rusty Russell [EMAIL PROTECTED] Date: Thu, 20 Dec 2007 18:53:48 +1100 Manipulating the magic chains is horrible; it looks simple to the places which simply want to iterate through it, but it's awful for code which wants to create them. I'm not saying complexity is inherent in this stuff, but assuming that it is the complexity should live as far away from the minions (the iterators in this case). Therefore, the creators is the right spot for the hard stuff. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
