Re: [patch] scsi: use lock per host instead of per device for shared queue tag host
Ed Lin wrote: There may possibly be some other errors. So we need a lock here. I think the simple but reliable way to do it is just to replace queue lock with a host lock. James pointed out that there may be performance slow down when many devices are accessed at the same time. But I think the major part is still on the hardware, and a host lock is the price these kind of controllers must pay. I agree. Further, a host lock is (a) common across many controllers, to protect host-wide resources and (b) only limits us when the controller is CPU-limited, a very rare scenario. Jeff - 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] scsi: use lock per host instead of per device for shared queue tag host
> -Original Message-
> From: Jens Axboe [mailto:[EMAIL PROTECTED]
> Sent: Thursday, January 25, 2007 7:48 AM
> To: Ed Lin
> Cc: David Somayajulu; Michael Reed; linux-scsi; linux-kernel;
> james.Bottomley; jeff; Promise_Linux
> Subject: Re: [patch] scsi: use lock per host instead of per
> device for shared queue tag host
>
>
> On Thu, Jan 25 2007, Jens Axboe wrote:
> > On Wed, Jan 24 2007, Ed Lin wrote:
> > >
> > >
> > > > -Original Message-
> > > > From: David Somayajulu [mailto:[EMAIL PROTECTED]
> > > > Sent: Wednesday, January 24, 2007 5:03 PM
> > > > To: Ed Lin; Michael Reed
> > > > Cc: linux-scsi; linux-kernel; james.Bottomley; jeff;
> > > > Promise_Linux; Jens Axboe
> > > > Subject: RE: [patch] scsi: use lock per host instead of per
> > > > device for shared queue tag host
> > > >
> > > >
> > > > > It seems another driver(qla4xxx) is also using shared
> queue tag.
> > > > > It is natural to imagine there might be same symptom in that
> > > > > driver. But I don't know the driver and have no hardware so I
> > > > > can not say anything certain about it.
> > > >
> > > > qla4xxx implements slightly differently, in the sense we
> > > > don't have the
> > > > equivalent of
> > > > struct st_ccb ccb[MU_MAX_REQUEST];
> > > > which is in struct st_hba. In other words we don't have
> a local array
> > > > which like stex to keep track of the outstanding
> commands to the hba.
> > > >
> > > > We had a discussion on this one while implementing
> block-layer tagging
> > > > in qla4xxx and Jens Axboe added the test_and_set_bit() in the
> > > > following
> > > > code in blk_queue_start_tag() to take care of it.
> > > > do {
> > > > tag = find_first_zero_bit(bqt->tag_map,
> bqt->max_depth);
> > > > if (tag >= bqt->max_depth)
> > > > return 1;
> > > > } while (test_and_set_bit(tag, bqt->tag_map));
> > > > Please see the following link for the discussion
> > > > http://marc.theaimsgroup.com/?l=linux-scsi=115886351206726=2
> > > >
> > > > Cheers
> > > > David Somayajulu
> > > > QLogic Corporation
> > > >
> > >
> > > Yes, this piece of code of allocating tag, in itself, is safe.
> > > But the following
> > >
> > > if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
> > > printk(KERN_ERR "%s: attempt to clear non-busy tag
> > > (%d)\n",
> > > __FUNCTION__, tag);
> > > return;
> > > }
> > >
> > > code of freeing tag (in blk_queue_end_tag())seems to be using
> > > unsafe __test_and_clear_bit instead of test_and_clear_bit.
> > > I once changed it to test_and_clear_bit and thought it was fixed.
> > > But the panic happened thereafter nonetheless(using gcc 3.4.6.
> > > gcc 4.1.0 is better but still with kernel errors). bqt also needs
> > > to be protected in this case. Replacing queue lock per device with
> > > a host lock is a simple but logical fix for it. To introduce a
> > > more refined lock is possible, but seems too tedious and elaborate
> > > for this issue, since a queue lock is already out there, and a
> > > hostwide lock is needed anyway.
> >
> > Does this fix it? There really should be no need to add
> extra locking
> > for this, it would be a shame.
> >
> > diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
> > index fb67897..e752e5d 100644
> > --- a/block/ll_rw_blk.c
> > +++ b/block/ll_rw_blk.c
> > @@ -1072,12 +1072,16 @@ void
> blk_queue_end_tag(request_queue_t *q, struct request *rq)
> > */
> > return;
> >
> > - if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
> > + smp_mb__before_clear_bit();
> > +
> > + if (unlikely(!test_and_clear_bit(tag, bqt->tag_map))) {
> > printk(KERN_ERR "%s: attempt to clear non-busy
> tag (%d)\n",
> >__FUNCTION__, tag);
> > return;
> > }
> >
> > + smp_mb__after_clear_bit();
> > +
> > list_del_init(>queuelist);
> > rq->cmd_flags &= ~REQ_QUEUED;
> > rq->tag = -1;
> &g
Re: [patch] scsi: use lock per host instead of per device for shared queue tag host
On Thu, Jan 25 2007, Jens Axboe wrote:
> On Wed, Jan 24 2007, Ed Lin wrote:
> >
> >
> > > -Original Message-
> > > From: David Somayajulu [mailto:[EMAIL PROTECTED]
> > > Sent: Wednesday, January 24, 2007 5:03 PM
> > > To: Ed Lin; Michael Reed
> > > Cc: linux-scsi; linux-kernel; james.Bottomley; jeff;
> > > Promise_Linux; Jens Axboe
> > > Subject: RE: [patch] scsi: use lock per host instead of per
> > > device for shared queue tag host
> > >
> > >
> > > > It seems another driver(qla4xxx) is also using shared queue tag.
> > > > It is natural to imagine there might be same symptom in that
> > > > driver. But I don't know the driver and have no hardware so I
> > > > can not say anything certain about it.
> > >
> > > qla4xxx implements slightly differently, in the sense we
> > > don't have the
> > > equivalent of
> > > struct st_ccb ccb[MU_MAX_REQUEST];
> > > which is in struct st_hba. In other words we don't have a local array
> > > which like stex to keep track of the outstanding commands to the hba.
> > >
> > > We had a discussion on this one while implementing block-layer tagging
> > > in qla4xxx and Jens Axboe added the test_and_set_bit() in the
> > > following
> > > code in blk_queue_start_tag() to take care of it.
> > > do {
> > > tag = find_first_zero_bit(bqt->tag_map, bqt->max_depth);
> > > if (tag >= bqt->max_depth)
> > > return 1;
> > > } while (test_and_set_bit(tag, bqt->tag_map));
> > > Please see the following link for the discussion
> > > http://marc.theaimsgroup.com/?l=linux-scsi=115886351206726=2
> > >
> > > Cheers
> > > David Somayajulu
> > > QLogic Corporation
> > >
> >
> > Yes, this piece of code of allocating tag, in itself, is safe.
> > But the following
> >
> > if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
> > printk(KERN_ERR "%s: attempt to clear non-busy tag
> > (%d)\n",
> >__FUNCTION__, tag);
> > return;
> > }
> >
> > code of freeing tag (in blk_queue_end_tag())seems to be using
> > unsafe __test_and_clear_bit instead of test_and_clear_bit.
> > I once changed it to test_and_clear_bit and thought it was fixed.
> > But the panic happened thereafter nonetheless(using gcc 3.4.6.
> > gcc 4.1.0 is better but still with kernel errors). bqt also needs
> > to be protected in this case. Replacing queue lock per device with
> > a host lock is a simple but logical fix for it. To introduce a
> > more refined lock is possible, but seems too tedious and elaborate
> > for this issue, since a queue lock is already out there, and a
> > hostwide lock is needed anyway.
>
> Does this fix it? There really should be no need to add extra locking
> for this, it would be a shame.
>
> diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
> index fb67897..e752e5d 100644
> --- a/block/ll_rw_blk.c
> +++ b/block/ll_rw_blk.c
> @@ -1072,12 +1072,16 @@ void blk_queue_end_tag(request_queue_t *q, struct
> request *rq)
>*/
> return;
>
> - if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
> + smp_mb__before_clear_bit();
> +
> + if (unlikely(!test_and_clear_bit(tag, bqt->tag_map))) {
> printk(KERN_ERR "%s: attempt to clear non-busy tag (%d)\n",
> __FUNCTION__, tag);
> return;
> }
>
> + smp_mb__after_clear_bit();
> +
> list_del_init(>queuelist);
> rq->cmd_flags &= ~REQ_QUEUED;
> rq->tag = -1;
>
Double checking the actual implementation, the smp_mb__* should not be
needed with the test_and_*_bit operations. The __test_and_clear_bit()
change is needed, though. What kind of crash did you see when you did
that? It should not crash, but you could see the "attempt to clear
non-busy tag" error though.
--
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] scsi: use lock per host instead of per device for shared queue tag host
On Wed, Jan 24 2007, Ed Lin wrote:
>
>
> > -Original Message-
> > From: David Somayajulu [mailto:[EMAIL PROTECTED]
> > Sent: Wednesday, January 24, 2007 5:03 PM
> > To: Ed Lin; Michael Reed
> > Cc: linux-scsi; linux-kernel; james.Bottomley; jeff;
> > Promise_Linux; Jens Axboe
> > Subject: RE: [patch] scsi: use lock per host instead of per
> > device for shared queue tag host
> >
> >
> > > It seems another driver(qla4xxx) is also using shared queue tag.
> > > It is natural to imagine there might be same symptom in that
> > > driver. But I don't know the driver and have no hardware so I
> > > can not say anything certain about it.
> >
> > qla4xxx implements slightly differently, in the sense we
> > don't have the
> > equivalent of
> > struct st_ccb ccb[MU_MAX_REQUEST];
> > which is in struct st_hba. In other words we don't have a local array
> > which like stex to keep track of the outstanding commands to the hba.
> >
> > We had a discussion on this one while implementing block-layer tagging
> > in qla4xxx and Jens Axboe added the test_and_set_bit() in the
> > following
> > code in blk_queue_start_tag() to take care of it.
> > do {
> > tag = find_first_zero_bit(bqt->tag_map, bqt->max_depth);
> > if (tag >= bqt->max_depth)
> > return 1;
> > } while (test_and_set_bit(tag, bqt->tag_map));
> > Please see the following link for the discussion
> > http://marc.theaimsgroup.com/?l=linux-scsi=115886351206726=2
> >
> > Cheers
> > David Somayajulu
> > QLogic Corporation
> >
>
> Yes, this piece of code of allocating tag, in itself, is safe.
> But the following
>
> if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
> printk(KERN_ERR "%s: attempt to clear non-busy tag
> (%d)\n",
> __FUNCTION__, tag);
> return;
> }
>
> code of freeing tag (in blk_queue_end_tag())seems to be using
> unsafe __test_and_clear_bit instead of test_and_clear_bit.
> I once changed it to test_and_clear_bit and thought it was fixed.
> But the panic happened thereafter nonetheless(using gcc 3.4.6.
> gcc 4.1.0 is better but still with kernel errors). bqt also needs
> to be protected in this case. Replacing queue lock per device with
> a host lock is a simple but logical fix for it. To introduce a
> more refined lock is possible, but seems too tedious and elaborate
> for this issue, since a queue lock is already out there, and a
> hostwide lock is needed anyway.
Does this fix it? There really should be no need to add extra locking
for this, it would be a shame.
diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index fb67897..e752e5d 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -1072,12 +1072,16 @@ void blk_queue_end_tag(request_queue_t *q, struct
request *rq)
*/
return;
- if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
+ smp_mb__before_clear_bit();
+
+ if (unlikely(!test_and_clear_bit(tag, bqt->tag_map))) {
printk(KERN_ERR "%s: attempt to clear non-busy tag (%d)\n",
__FUNCTION__, tag);
return;
}
+ smp_mb__after_clear_bit();
+
list_del_init(>queuelist);
rq->cmd_flags &= ~REQ_QUEUED;
rq->tag = -1;
--
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] scsi: use lock per host instead of per device for shared queue tag host
On Wed, Jan 24 2007, Ed Lin wrote:
-Original Message-
From: David Somayajulu [mailto:[EMAIL PROTECTED]
Sent: Wednesday, January 24, 2007 5:03 PM
To: Ed Lin; Michael Reed
Cc: linux-scsi; linux-kernel; james.Bottomley; jeff;
Promise_Linux; Jens Axboe
Subject: RE: [patch] scsi: use lock per host instead of per
device for shared queue tag host
It seems another driver(qla4xxx) is also using shared queue tag.
It is natural to imagine there might be same symptom in that
driver. But I don't know the driver and have no hardware so I
can not say anything certain about it.
qla4xxx implements slightly differently, in the sense we
don't have the
equivalent of
struct st_ccb ccb[MU_MAX_REQUEST];
which is in struct st_hba. In other words we don't have a local array
which like stex to keep track of the outstanding commands to the hba.
We had a discussion on this one while implementing block-layer tagging
in qla4xxx and Jens Axboe added the test_and_set_bit() in the
following
code in blk_queue_start_tag() to take care of it.
do {
tag = find_first_zero_bit(bqt-tag_map, bqt-max_depth);
if (tag = bqt-max_depth)
return 1;
} while (test_and_set_bit(tag, bqt-tag_map));
Please see the following link for the discussion
http://marc.theaimsgroup.com/?l=linux-scsim=115886351206726w=2
Cheers
David Somayajulu
QLogic Corporation
Yes, this piece of code of allocating tag, in itself, is safe.
But the following
if (unlikely(!__test_and_clear_bit(tag, bqt-tag_map))) {
printk(KERN_ERR %s: attempt to clear non-busy tag
(%d)\n,
__FUNCTION__, tag);
return;
}
code of freeing tag (in blk_queue_end_tag())seems to be using
unsafe __test_and_clear_bit instead of test_and_clear_bit.
I once changed it to test_and_clear_bit and thought it was fixed.
But the panic happened thereafter nonetheless(using gcc 3.4.6.
gcc 4.1.0 is better but still with kernel errors). bqt also needs
to be protected in this case. Replacing queue lock per device with
a host lock is a simple but logical fix for it. To introduce a
more refined lock is possible, but seems too tedious and elaborate
for this issue, since a queue lock is already out there, and a
hostwide lock is needed anyway.
Does this fix it? There really should be no need to add extra locking
for this, it would be a shame.
diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index fb67897..e752e5d 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -1072,12 +1072,16 @@ void blk_queue_end_tag(request_queue_t *q, struct
request *rq)
*/
return;
- if (unlikely(!__test_and_clear_bit(tag, bqt-tag_map))) {
+ smp_mb__before_clear_bit();
+
+ if (unlikely(!test_and_clear_bit(tag, bqt-tag_map))) {
printk(KERN_ERR %s: attempt to clear non-busy tag (%d)\n,
__FUNCTION__, tag);
return;
}
+ smp_mb__after_clear_bit();
+
list_del_init(rq-queuelist);
rq-cmd_flags = ~REQ_QUEUED;
rq-tag = -1;
--
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] scsi: use lock per host instead of per device for shared queue tag host
On Thu, Jan 25 2007, Jens Axboe wrote:
On Wed, Jan 24 2007, Ed Lin wrote:
-Original Message-
From: David Somayajulu [mailto:[EMAIL PROTECTED]
Sent: Wednesday, January 24, 2007 5:03 PM
To: Ed Lin; Michael Reed
Cc: linux-scsi; linux-kernel; james.Bottomley; jeff;
Promise_Linux; Jens Axboe
Subject: RE: [patch] scsi: use lock per host instead of per
device for shared queue tag host
It seems another driver(qla4xxx) is also using shared queue tag.
It is natural to imagine there might be same symptom in that
driver. But I don't know the driver and have no hardware so I
can not say anything certain about it.
qla4xxx implements slightly differently, in the sense we
don't have the
equivalent of
struct st_ccb ccb[MU_MAX_REQUEST];
which is in struct st_hba. In other words we don't have a local array
which like stex to keep track of the outstanding commands to the hba.
We had a discussion on this one while implementing block-layer tagging
in qla4xxx and Jens Axboe added the test_and_set_bit() in the
following
code in blk_queue_start_tag() to take care of it.
do {
tag = find_first_zero_bit(bqt-tag_map, bqt-max_depth);
if (tag = bqt-max_depth)
return 1;
} while (test_and_set_bit(tag, bqt-tag_map));
Please see the following link for the discussion
http://marc.theaimsgroup.com/?l=linux-scsim=115886351206726w=2
Cheers
David Somayajulu
QLogic Corporation
Yes, this piece of code of allocating tag, in itself, is safe.
But the following
if (unlikely(!__test_and_clear_bit(tag, bqt-tag_map))) {
printk(KERN_ERR %s: attempt to clear non-busy tag
(%d)\n,
__FUNCTION__, tag);
return;
}
code of freeing tag (in blk_queue_end_tag())seems to be using
unsafe __test_and_clear_bit instead of test_and_clear_bit.
I once changed it to test_and_clear_bit and thought it was fixed.
But the panic happened thereafter nonetheless(using gcc 3.4.6.
gcc 4.1.0 is better but still with kernel errors). bqt also needs
to be protected in this case. Replacing queue lock per device with
a host lock is a simple but logical fix for it. To introduce a
more refined lock is possible, but seems too tedious and elaborate
for this issue, since a queue lock is already out there, and a
hostwide lock is needed anyway.
Does this fix it? There really should be no need to add extra locking
for this, it would be a shame.
diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index fb67897..e752e5d 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -1072,12 +1072,16 @@ void blk_queue_end_tag(request_queue_t *q, struct
request *rq)
*/
return;
- if (unlikely(!__test_and_clear_bit(tag, bqt-tag_map))) {
+ smp_mb__before_clear_bit();
+
+ if (unlikely(!test_and_clear_bit(tag, bqt-tag_map))) {
printk(KERN_ERR %s: attempt to clear non-busy tag (%d)\n,
__FUNCTION__, tag);
return;
}
+ smp_mb__after_clear_bit();
+
list_del_init(rq-queuelist);
rq-cmd_flags = ~REQ_QUEUED;
rq-tag = -1;
Double checking the actual implementation, the smp_mb__* should not be
needed with the test_and_*_bit operations. The __test_and_clear_bit()
change is needed, though. What kind of crash did you see when you did
that? It should not crash, but you could see the attempt to clear
non-busy tag error though.
--
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] scsi: use lock per host instead of per device for shared queue tag host
-Original Message-
From: Jens Axboe [mailto:[EMAIL PROTECTED]
Sent: Thursday, January 25, 2007 7:48 AM
To: Ed Lin
Cc: David Somayajulu; Michael Reed; linux-scsi; linux-kernel;
james.Bottomley; jeff; Promise_Linux
Subject: Re: [patch] scsi: use lock per host instead of per
device for shared queue tag host
On Thu, Jan 25 2007, Jens Axboe wrote:
On Wed, Jan 24 2007, Ed Lin wrote:
-Original Message-
From: David Somayajulu [mailto:[EMAIL PROTECTED]
Sent: Wednesday, January 24, 2007 5:03 PM
To: Ed Lin; Michael Reed
Cc: linux-scsi; linux-kernel; james.Bottomley; jeff;
Promise_Linux; Jens Axboe
Subject: RE: [patch] scsi: use lock per host instead of per
device for shared queue tag host
It seems another driver(qla4xxx) is also using shared
queue tag.
It is natural to imagine there might be same symptom in that
driver. But I don't know the driver and have no hardware so I
can not say anything certain about it.
qla4xxx implements slightly differently, in the sense we
don't have the
equivalent of
struct st_ccb ccb[MU_MAX_REQUEST];
which is in struct st_hba. In other words we don't have
a local array
which like stex to keep track of the outstanding
commands to the hba.
We had a discussion on this one while implementing
block-layer tagging
in qla4xxx and Jens Axboe added the test_and_set_bit() in the
following
code in blk_queue_start_tag() to take care of it.
do {
tag = find_first_zero_bit(bqt-tag_map,
bqt-max_depth);
if (tag = bqt-max_depth)
return 1;
} while (test_and_set_bit(tag, bqt-tag_map));
Please see the following link for the discussion
http://marc.theaimsgroup.com/?l=linux-scsim=115886351206726w=2
Cheers
David Somayajulu
QLogic Corporation
Yes, this piece of code of allocating tag, in itself, is safe.
But the following
if (unlikely(!__test_and_clear_bit(tag, bqt-tag_map))) {
printk(KERN_ERR %s: attempt to clear non-busy tag
(%d)\n,
__FUNCTION__, tag);
return;
}
code of freeing tag (in blk_queue_end_tag())seems to be using
unsafe __test_and_clear_bit instead of test_and_clear_bit.
I once changed it to test_and_clear_bit and thought it was fixed.
But the panic happened thereafter nonetheless(using gcc 3.4.6.
gcc 4.1.0 is better but still with kernel errors). bqt also needs
to be protected in this case. Replacing queue lock per device with
a host lock is a simple but logical fix for it. To introduce a
more refined lock is possible, but seems too tedious and elaborate
for this issue, since a queue lock is already out there, and a
hostwide lock is needed anyway.
Does this fix it? There really should be no need to add
extra locking
for this, it would be a shame.
diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index fb67897..e752e5d 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -1072,12 +1072,16 @@ void
blk_queue_end_tag(request_queue_t *q, struct request *rq)
*/
return;
- if (unlikely(!__test_and_clear_bit(tag, bqt-tag_map))) {
+ smp_mb__before_clear_bit();
+
+ if (unlikely(!test_and_clear_bit(tag, bqt-tag_map))) {
printk(KERN_ERR %s: attempt to clear non-busy
tag (%d)\n,
__FUNCTION__, tag);
return;
}
+ smp_mb__after_clear_bit();
+
list_del_init(rq-queuelist);
rq-cmd_flags = ~REQ_QUEUED;
rq-tag = -1;
Double checking the actual implementation, the smp_mb__* should not be
needed with the test_and_*_bit operations. The __test_and_clear_bit()
change is needed, though. What kind of crash did you see when you did
that? It should not crash, but you could see the attempt to clear
non-busy tag error though.
Besides the test_and_clear_bit, I think the bqt code(refer to last mail)
also needs protection, like:
list_del_init(rq-queuelist);
...
if (unlikely(bqt-tag_index[tag] == NULL))
printk(KERN_ERR %s: tag %d is missing\n,
__FUNCTION__, tag);
bqt-tag_index[tag] = NULL;
bqt-busy--;
and
bqt-tag_index[tag] = rq;
...
list_add(rq-queuelist, bqt-busy_list);
bqt-busy++;
because bqt is also globally shared within all devices in the host in
this case. (q-queue_tags was assigned as host-bqt in scsi_activate_tcq
)
With a gcc 4.1.0 compiled kernel, I did not get kernel panic, but
still got kernel errors: tag is missing. I guess a possible race
scenario could be:
cpu a:__test_and_clear_bit
cpu b:test_and_set_bit, allocate a tag just freed by cpu a
cpu b:bqt-tag_index[tag] = rq;
cpu a:bqt-tag_index[tag] = NULL;
Next time, when the request
Re: [patch] scsi: use lock per host instead of per device for shared queue tag host
Ed Lin wrote: There may possibly be some other errors. So we need a lock here. I think the simple but reliable way to do it is just to replace queue lock with a host lock. James pointed out that there may be performance slow down when many devices are accessed at the same time. But I think the major part is still on the hardware, and a host lock is the price these kind of controllers must pay. I agree. Further, a host lock is (a) common across many controllers, to protect host-wide resources and (b) only limits us when the controller is CPU-limited, a very rare scenario. Jeff - 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] scsi: use lock per host instead of per device for shared queue tag host
> -Original Message-
> From: David Somayajulu [mailto:[EMAIL PROTECTED]
> Sent: Wednesday, January 24, 2007 5:03 PM
> To: Ed Lin; Michael Reed
> Cc: linux-scsi; linux-kernel; james.Bottomley; jeff;
> Promise_Linux; Jens Axboe
> Subject: RE: [patch] scsi: use lock per host instead of per
> device for shared queue tag host
>
>
> > It seems another driver(qla4xxx) is also using shared queue tag.
> > It is natural to imagine there might be same symptom in that
> > driver. But I don't know the driver and have no hardware so I
> > can not say anything certain about it.
>
> qla4xxx implements slightly differently, in the sense we
> don't have the
> equivalent of
> struct st_ccb ccb[MU_MAX_REQUEST];
> which is in struct st_hba. In other words we don't have a local array
> which like stex to keep track of the outstanding commands to the hba.
>
> We had a discussion on this one while implementing block-layer tagging
> in qla4xxx and Jens Axboe added the test_and_set_bit() in the
> following
> code in blk_queue_start_tag() to take care of it.
> do {
> tag = find_first_zero_bit(bqt->tag_map, bqt->max_depth);
> if (tag >= bqt->max_depth)
> return 1;
> } while (test_and_set_bit(tag, bqt->tag_map));
> Please see the following link for the discussion
> http://marc.theaimsgroup.com/?l=linux-scsi=115886351206726=2
>
> Cheers
> David Somayajulu
> QLogic Corporation
>
Yes, this piece of code of allocating tag, in itself, is safe.
But the following
if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
printk(KERN_ERR "%s: attempt to clear non-busy tag
(%d)\n",
__FUNCTION__, tag);
return;
}
code of freeing tag (in blk_queue_end_tag())seems to be using
unsafe __test_and_clear_bit instead of test_and_clear_bit.
I once changed it to test_and_clear_bit and thought it was fixed.
But the panic happened thereafter nonetheless(using gcc 3.4.6.
gcc 4.1.0 is better but still with kernel errors). bqt also needs
to be protected in this case. Replacing queue lock per device with
a host lock is a simple but logical fix for it. To introduce a
more refined lock is possible, but seems too tedious and elaborate
for this issue, since a queue lock is already out there, and a
hostwide lock is needed anyway.
Thanks,
Ed Lin
-
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] scsi: use lock per host instead of per device for shared queue tag host
> It seems another driver(qla4xxx) is also using shared queue tag.
> It is natural to imagine there might be same symptom in that
> driver. But I don't know the driver and have no hardware so I
> can not say anything certain about it.
qla4xxx implements slightly differently, in the sense we don't have the
equivalent of
struct st_ccb ccb[MU_MAX_REQUEST];
which is in struct st_hba. In other words we don't have a local array
which like stex to keep track of the outstanding commands to the hba.
We had a discussion on this one while implementing block-layer tagging
in qla4xxx and Jens Axboe added the test_and_set_bit() in the following
code in blk_queue_start_tag() to take care of it.
do {
tag = find_first_zero_bit(bqt->tag_map, bqt->max_depth);
if (tag >= bqt->max_depth)
return 1;
} while (test_and_set_bit(tag, bqt->tag_map));
Please see the following link for the discussion
http://marc.theaimsgroup.com/?l=linux-scsi=115886351206726=2
Cheers
David Somayajulu
QLogic Corporation
-
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] scsi: use lock per host instead of per device for shared queue tag host
> -Original Message- > From: Michael Reed [mailto:[EMAIL PROTECTED] > Sent: Wednesday, January 24, 2007 7:59 AM > To: Ed Lin > Cc: linux-scsi; linux-kernel; james.Bottomley; jeff; Promise_Linux > Subject: Re: [patch] scsi: use lock per host instead of per > device for shared queue tag host > > > How 'bout a comment in scsh_host.h indicating that the > pointer will be NULL unless > initialized by the driver? > > "Protect shared block queue tag" is unique to stex. Perhaps > have no comment on > the variable declaration in scsi_host.h and explain why you > use it in stex. > > Mike > > Thanks for commenting. I agree more detailed explaination should be better. It seems another driver(qla4xxx) is also using shared queue tag. It is natural to imagine there might be same symptom in that driver. But I don't know the driver and have no hardware so I can not say anything certain about it. Ed Lin - 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] scsi: use lock per host instead of per device for shared queue tag host
On Tue, 2007-01-23 at 16:53 -0800, Ed Lin wrote: > The block layer uses lock to protect request queue. Every scsi device > has a unique request queue, and queue lock is the default lock in struct > request_queue. This is good for normal cases. But for a host with > shared queue tag (e.g. stex controllers), a queue lock per device means > the shared queue tag is not protected when multiple devices are accessed > at a same time. This patch is a simple fix for this situation by introducing > a host queue lock to protect shared queue tag. Without this patch we will > see various kernel panics (including the BUG() and kernel errors in > blk_queue_start_tag and blk_queue_end_tag of ll_rw_blk.c) when accessing > different devices simultaneously (e.g. copying big file from one device to > another in smp kernels). This patch looks OK in principle. However, are you sure you're not using a sledgehammer to crack a nut? If the only reason you're doing this is because of the shared tag map, then probably that should be the area you protect with a per-tag-map lock. The net effect of what you've done will be to serialise all accesses to your storage devices. For a small number of devices, this probably won't matter than much, but for large numbers of devices, you're probably going to introduce artificial performance degredation in the I/O scheduler. James - 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] scsi: use lock per host instead of per device for shared queue tag host
How 'bout a comment in scsh_host.h indicating that the pointer will be NULL
unless
initialized by the driver?
"Protect shared block queue tag" is unique to stex. Perhaps have no comment on
the variable declaration in scsi_host.h and explain why you use it in stex.
Mike
Ed Lin wrote:
> The block layer uses lock to protect request queue. Every scsi device
> has a unique request queue, and queue lock is the default lock in struct
> request_queue. This is good for normal cases. But for a host with
> shared queue tag (e.g. stex controllers), a queue lock per device means
> the shared queue tag is not protected when multiple devices are accessed
> at a same time. This patch is a simple fix for this situation by introducing
> a host queue lock to protect shared queue tag. Without this patch we will
> see various kernel panics (including the BUG() and kernel errors in
> blk_queue_start_tag and blk_queue_end_tag of ll_rw_blk.c) when accessing
> different devices simultaneously (e.g. copying big file from one device to
> another in smp kernels).
>
> This is against kernel 2.6.20-rc5.
>
> Signed-off-by: Ed Lin <[EMAIL PROTECTED]>
> ---
> drivers/scsi/scsi_lib.c |2 +-
> drivers/scsi/stex.c |2 ++
> include/scsi/scsi_host.h |3 +++
> 3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff -purN a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> --- a/drivers/scsi/scsi_lib.c 2007-01-23 14:40:28.0 -0800
> +++ b/drivers/scsi/scsi_lib.c 2007-01-23 14:46:43.0 -0800
> @@ -1574,7 +1574,7 @@ struct request_queue *__scsi_alloc_queue
> {
> struct request_queue *q;
>
> - q = blk_init_queue(request_fn, NULL);
> + q = blk_init_queue(request_fn, shost->req_q_lock);
> if (!q)
> return NULL;
>
> diff -purN a/drivers/scsi/stex.c b/drivers/scsi/stex.c
> --- a/drivers/scsi/stex.c 2007-01-23 14:40:28.0 -0800
> +++ b/drivers/scsi/stex.c 2007-01-23 14:48:59.0 -0800
> @@ -1254,6 +1254,8 @@ stex_probe(struct pci_dev *pdev, const s
> if (err)
> goto out_free_irq;
>
> + spin_lock_init(>__req_q_lock);
> + host->req_q_lock = >__req_q_lock;
> err = scsi_init_shared_tag_map(host, host->can_queue);
> if (err) {
> printk(KERN_ERR DRV_NAME "(%s): init shared queue failed\n",
> diff -purN a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> --- a/include/scsi/scsi_host.h2007-01-23 14:40:29.0 -0800
> +++ b/include/scsi/scsi_host.h2007-01-23 14:57:04.0 -0800
> @@ -508,6 +508,9 @@ struct Scsi_Host {
> spinlock_t default_lock;
> spinlock_t *host_lock;
>
> + spinlock_t __req_q_lock;
> + spinlock_t *req_q_lock;/* protect shared block queue tag */
> +
> struct mutexscan_mutex;/* serialize scanning activity */
>
> struct list_headeh_cmd_q;
>
>
>
> -
> 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/
-
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] scsi: use lock per host instead of per device for shared queue tag host
How 'bout a comment in scsh_host.h indicating that the pointer will be NULL
unless
initialized by the driver?
Protect shared block queue tag is unique to stex. Perhaps have no comment on
the variable declaration in scsi_host.h and explain why you use it in stex.
Mike
Ed Lin wrote:
The block layer uses lock to protect request queue. Every scsi device
has a unique request queue, and queue lock is the default lock in struct
request_queue. This is good for normal cases. But for a host with
shared queue tag (e.g. stex controllers), a queue lock per device means
the shared queue tag is not protected when multiple devices are accessed
at a same time. This patch is a simple fix for this situation by introducing
a host queue lock to protect shared queue tag. Without this patch we will
see various kernel panics (including the BUG() and kernel errors in
blk_queue_start_tag and blk_queue_end_tag of ll_rw_blk.c) when accessing
different devices simultaneously (e.g. copying big file from one device to
another in smp kernels).
This is against kernel 2.6.20-rc5.
Signed-off-by: Ed Lin [EMAIL PROTECTED]
---
drivers/scsi/scsi_lib.c |2 +-
drivers/scsi/stex.c |2 ++
include/scsi/scsi_host.h |3 +++
3 files changed, 6 insertions(+), 1 deletion(-)
diff -purN a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c 2007-01-23 14:40:28.0 -0800
+++ b/drivers/scsi/scsi_lib.c 2007-01-23 14:46:43.0 -0800
@@ -1574,7 +1574,7 @@ struct request_queue *__scsi_alloc_queue
{
struct request_queue *q;
- q = blk_init_queue(request_fn, NULL);
+ q = blk_init_queue(request_fn, shost-req_q_lock);
if (!q)
return NULL;
diff -purN a/drivers/scsi/stex.c b/drivers/scsi/stex.c
--- a/drivers/scsi/stex.c 2007-01-23 14:40:28.0 -0800
+++ b/drivers/scsi/stex.c 2007-01-23 14:48:59.0 -0800
@@ -1254,6 +1254,8 @@ stex_probe(struct pci_dev *pdev, const s
if (err)
goto out_free_irq;
+ spin_lock_init(host-__req_q_lock);
+ host-req_q_lock = host-__req_q_lock;
err = scsi_init_shared_tag_map(host, host-can_queue);
if (err) {
printk(KERN_ERR DRV_NAME (%s): init shared queue failed\n,
diff -purN a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h2007-01-23 14:40:29.0 -0800
+++ b/include/scsi/scsi_host.h2007-01-23 14:57:04.0 -0800
@@ -508,6 +508,9 @@ struct Scsi_Host {
spinlock_t default_lock;
spinlock_t *host_lock;
+ spinlock_t __req_q_lock;
+ spinlock_t *req_q_lock;/* protect shared block queue tag */
+
struct mutexscan_mutex;/* serialize scanning activity */
struct list_headeh_cmd_q;
-
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/
-
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] scsi: use lock per host instead of per device for shared queue tag host
On Tue, 2007-01-23 at 16:53 -0800, Ed Lin wrote: The block layer uses lock to protect request queue. Every scsi device has a unique request queue, and queue lock is the default lock in struct request_queue. This is good for normal cases. But for a host with shared queue tag (e.g. stex controllers), a queue lock per device means the shared queue tag is not protected when multiple devices are accessed at a same time. This patch is a simple fix for this situation by introducing a host queue lock to protect shared queue tag. Without this patch we will see various kernel panics (including the BUG() and kernel errors in blk_queue_start_tag and blk_queue_end_tag of ll_rw_blk.c) when accessing different devices simultaneously (e.g. copying big file from one device to another in smp kernels). This patch looks OK in principle. However, are you sure you're not using a sledgehammer to crack a nut? If the only reason you're doing this is because of the shared tag map, then probably that should be the area you protect with a per-tag-map lock. The net effect of what you've done will be to serialise all accesses to your storage devices. For a small number of devices, this probably won't matter than much, but for large numbers of devices, you're probably going to introduce artificial performance degredation in the I/O scheduler. James - 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] scsi: use lock per host instead of per device for shared queue tag host
-Original Message- From: Michael Reed [mailto:[EMAIL PROTECTED] Sent: Wednesday, January 24, 2007 7:59 AM To: Ed Lin Cc: linux-scsi; linux-kernel; james.Bottomley; jeff; Promise_Linux Subject: Re: [patch] scsi: use lock per host instead of per device for shared queue tag host How 'bout a comment in scsh_host.h indicating that the pointer will be NULL unless initialized by the driver? Protect shared block queue tag is unique to stex. Perhaps have no comment on the variable declaration in scsi_host.h and explain why you use it in stex. Mike Thanks for commenting. I agree more detailed explaination should be better. It seems another driver(qla4xxx) is also using shared queue tag. It is natural to imagine there might be same symptom in that driver. But I don't know the driver and have no hardware so I can not say anything certain about it. Ed Lin - 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] scsi: use lock per host instead of per device for shared queue tag host
It seems another driver(qla4xxx) is also using shared queue tag.
It is natural to imagine there might be same symptom in that
driver. But I don't know the driver and have no hardware so I
can not say anything certain about it.
qla4xxx implements slightly differently, in the sense we don't have the
equivalent of
struct st_ccb ccb[MU_MAX_REQUEST];
which is in struct st_hba. In other words we don't have a local array
which like stex to keep track of the outstanding commands to the hba.
We had a discussion on this one while implementing block-layer tagging
in qla4xxx and Jens Axboe added the test_and_set_bit() in the following
code in blk_queue_start_tag() to take care of it.
do {
tag = find_first_zero_bit(bqt-tag_map, bqt-max_depth);
if (tag = bqt-max_depth)
return 1;
} while (test_and_set_bit(tag, bqt-tag_map));
Please see the following link for the discussion
http://marc.theaimsgroup.com/?l=linux-scsim=115886351206726w=2
Cheers
David Somayajulu
QLogic Corporation
-
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] scsi: use lock per host instead of per device for shared queue tag host
-Original Message-
From: David Somayajulu [mailto:[EMAIL PROTECTED]
Sent: Wednesday, January 24, 2007 5:03 PM
To: Ed Lin; Michael Reed
Cc: linux-scsi; linux-kernel; james.Bottomley; jeff;
Promise_Linux; Jens Axboe
Subject: RE: [patch] scsi: use lock per host instead of per
device for shared queue tag host
It seems another driver(qla4xxx) is also using shared queue tag.
It is natural to imagine there might be same symptom in that
driver. But I don't know the driver and have no hardware so I
can not say anything certain about it.
qla4xxx implements slightly differently, in the sense we
don't have the
equivalent of
struct st_ccb ccb[MU_MAX_REQUEST];
which is in struct st_hba. In other words we don't have a local array
which like stex to keep track of the outstanding commands to the hba.
We had a discussion on this one while implementing block-layer tagging
in qla4xxx and Jens Axboe added the test_and_set_bit() in the
following
code in blk_queue_start_tag() to take care of it.
do {
tag = find_first_zero_bit(bqt-tag_map, bqt-max_depth);
if (tag = bqt-max_depth)
return 1;
} while (test_and_set_bit(tag, bqt-tag_map));
Please see the following link for the discussion
http://marc.theaimsgroup.com/?l=linux-scsim=115886351206726w=2
Cheers
David Somayajulu
QLogic Corporation
Yes, this piece of code of allocating tag, in itself, is safe.
But the following
if (unlikely(!__test_and_clear_bit(tag, bqt-tag_map))) {
printk(KERN_ERR %s: attempt to clear non-busy tag
(%d)\n,
__FUNCTION__, tag);
return;
}
code of freeing tag (in blk_queue_end_tag())seems to be using
unsafe __test_and_clear_bit instead of test_and_clear_bit.
I once changed it to test_and_clear_bit and thought it was fixed.
But the panic happened thereafter nonetheless(using gcc 3.4.6.
gcc 4.1.0 is better but still with kernel errors). bqt also needs
to be protected in this case. Replacing queue lock per device with
a host lock is a simple but logical fix for it. To introduce a
more refined lock is possible, but seems too tedious and elaborate
for this issue, since a queue lock is already out there, and a
hostwide lock is needed anyway.
Thanks,
Ed Lin
-
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/
