| Message ID | 1481153486-5185-1-git-send-email-tyreld@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Dec 07, 2016 at 05:31:26PM -0600, Tyrel Datwyler wrote: > The first byte of each CRQ entry is used to indicate whether an entry is > a valid response or free for the VIOS to use. After processing a > response the driver sets the valid byte to zero to indicate the entry is > now free to be reused. Add a memory barrier after this write to ensure > no other stores are reordered when updating the valid byte. > > Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com> > --- > drivers/scsi/ibmvscsi/ibmvscsi.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c > index d9534ee..2f5b07e 100644 > --- a/drivers/scsi/ibmvscsi/ibmvscsi.c > +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c > @@ -232,6 +232,7 @@ static void ibmvscsi_task(void *data) > while ((crq = crq_queue_next_crq(&hostdata->queue)) != NULL) { > ibmvscsi_handle_crq(crq, hostdata); > crq->valid = VIOSRP_CRQ_FREE; > + wmb(); > } > > vio_enable_interrupts(vdev); > @@ -240,6 +241,7 @@ static void ibmvscsi_task(void *data) > vio_disable_interrupts(vdev); > ibmvscsi_handle_crq(crq, hostdata); > crq->valid = VIOSRP_CRQ_FREE; > + wmb(); > } else { > done = 1; > } Is this something you have seen in the wild or just a "better save than sorry" barrier? Thanks, Johannes
Reviewed-by: Brian King <brking@linux.vnet.ibm.com>
On 08/12/2016 00:31, Tyrel Datwyler wrote: > The first byte of each CRQ entry is used to indicate whether an entry is > a valid response or free for the VIOS to use. After processing a > response the driver sets the valid byte to zero to indicate the entry is > now free to be reused. Add a memory barrier after this write to ensure > no other stores are reordered when updating the valid byte. > > Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com> > --- > drivers/scsi/ibmvscsi/ibmvscsi.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c > index d9534ee..2f5b07e 100644 > --- a/drivers/scsi/ibmvscsi/ibmvscsi.c > +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c > @@ -232,6 +232,7 @@ static void ibmvscsi_task(void *data) > while ((crq = crq_queue_next_crq(&hostdata->queue)) != NULL) { > ibmvscsi_handle_crq(crq, hostdata); > crq->valid = VIOSRP_CRQ_FREE; > + wmb(); > } > > vio_enable_interrupts(vdev); > @@ -240,6 +241,7 @@ static void ibmvscsi_task(void *data) > vio_disable_interrupts(vdev); > ibmvscsi_handle_crq(crq, hostdata); > crq->valid = VIOSRP_CRQ_FREE; > + wmb(); Should this driver use virt_wmb instead? Paolo > } else { > done = 1; > } >
On 12/08/2016 03:29 PM, Paolo Bonzini wrote: > > > On 08/12/2016 00:31, Tyrel Datwyler wrote: >> The first byte of each CRQ entry is used to indicate whether an entry is >> a valid response or free for the VIOS to use. After processing a >> response the driver sets the valid byte to zero to indicate the entry is >> now free to be reused. Add a memory barrier after this write to ensure >> no other stores are reordered when updating the valid byte. >> >> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com> >> --- >> drivers/scsi/ibmvscsi/ibmvscsi.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c >> index d9534ee..2f5b07e 100644 >> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c >> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c >> @@ -232,6 +232,7 @@ static void ibmvscsi_task(void *data) >> while ((crq = crq_queue_next_crq(&hostdata->queue)) != NULL) { >> ibmvscsi_handle_crq(crq, hostdata); >> crq->valid = VIOSRP_CRQ_FREE; >> + wmb(); >> } >> >> vio_enable_interrupts(vdev); >> @@ -240,6 +241,7 @@ static void ibmvscsi_task(void *data) >> vio_disable_interrupts(vdev); >> ibmvscsi_handle_crq(crq, hostdata); >> crq->valid = VIOSRP_CRQ_FREE; >> + wmb(); > > Should this driver use virt_wmb instead? Both virt_wmb and wmb reduce to a lwsync instruction under PowerPC. -Tyrel > > Paolo > >> } else { >> done = 1; >> } >>
On 12/08/2016 01:06 AM, Johannes Thumshirn wrote: > On Wed, Dec 07, 2016 at 05:31:26PM -0600, Tyrel Datwyler wrote: >> The first byte of each CRQ entry is used to indicate whether an entry is >> a valid response or free for the VIOS to use. After processing a >> response the driver sets the valid byte to zero to indicate the entry is >> now free to be reused. Add a memory barrier after this write to ensure >> no other stores are reordered when updating the valid byte. >> >> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com> >> --- >> drivers/scsi/ibmvscsi/ibmvscsi.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c >> index d9534ee..2f5b07e 100644 >> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c >> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c >> @@ -232,6 +232,7 @@ static void ibmvscsi_task(void *data) >> while ((crq = crq_queue_next_crq(&hostdata->queue)) != NULL) { >> ibmvscsi_handle_crq(crq, hostdata); >> crq->valid = VIOSRP_CRQ_FREE; >> + wmb(); >> } >> >> vio_enable_interrupts(vdev); >> @@ -240,6 +241,7 @@ static void ibmvscsi_task(void *data) >> vio_disable_interrupts(vdev); >> ibmvscsi_handle_crq(crq, hostdata); >> crq->valid = VIOSRP_CRQ_FREE; >> + wmb(); >> } else { >> done = 1; >> } > > Is this something you have seen in the wild or just a "better save than sorry" > barrier? I myself have not observed or heard of anybody hitting an issue here. However, based on conversation with the VIOS developers, who have indicated it is required, this is a "better safe than sorry" scenario. Further, it matches what we already do in the ibmvfc driver for the CRQ processing logic. -Tyrel > > Thanks, > Johannes >
On Wed, 2016-12-07 at 17:31 -0600, Tyrel Datwyler wrote: > The first byte of each CRQ entry is used to indicate whether an entry is > a valid response or free for the VIOS to use. After processing a > response the driver sets the valid byte to zero to indicate the entry is > now free to be reused. Add a memory barrier after this write to ensure > no other stores are reordered when updating the valid byte. Which "other stores" specifically ? This smells fishy without that precision. It's important to always understand what exactly barriers order with. Cheers, Ben. > Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com> > --- > drivers/scsi/ibmvscsi/ibmvscsi.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c > index d9534ee..2f5b07e 100644 > --- a/drivers/scsi/ibmvscsi/ibmvscsi.c > +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c > @@ -232,6 +232,7 @@ static void ibmvscsi_task(void *data) > > while ((crq = crq_queue_next_crq(&hostdata->queue)) != NULL) { > > ibmvscsi_handle_crq(crq, hostdata); > > crq->valid = VIOSRP_CRQ_FREE; > > + wmb(); > > } > > > vio_enable_interrupts(vdev); > @@ -240,6 +241,7 @@ static void ibmvscsi_task(void *data) > > vio_disable_interrupts(vdev); > > ibmvscsi_handle_crq(crq, hostdata); > > crq->valid = VIOSRP_CRQ_FREE; > > + wmb(); > > } else { > > done = 1; > > }
On 12/09/2016 01:20 PM, Benjamin Herrenschmidt wrote: > On Wed, 2016-12-07 at 17:31 -0600, Tyrel Datwyler wrote: >> The first byte of each CRQ entry is used to indicate whether an entry is >> a valid response or free for the VIOS to use. After processing a >> response the driver sets the valid byte to zero to indicate the entry is >> now free to be reused. Add a memory barrier after this write to ensure >> no other stores are reordered when updating the valid byte. > > Which "other stores" specifically ? This smells fishy without that > precision. It's important to always understand what exactly barriers > order with. So, this patch initially came about while chasing a data integrity issue based on the observation that we were already doing this same write barrier in the virtual fibre channel driver. However, the more I stare at it I agree it does seem fishy and I can't see any other stores that we need to order with here. In terms of the 16byte CRQ entries we only ever write to the first byte as a sort of doorbell to tell the VIOS that we have processed the data and the entry is free to be re-used. The remainder of the CRQ is only ever read from. The wmb() in the ibmvfc driver was part of a commit from Brian that also involved a necessary rmb() that prevented stale data from load re-ordering by ensuring that a read from the first byte completed and contained a valid value prior to doing any other reads of the CRQ entry. I'd have to defer to Brian as to whether he remembers a legitimate reason for the wmb(). -Tyrel > > Cheers, > Ben. > >> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com> >> --- >> drivers/scsi/ibmvscsi/ibmvscsi.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c >> index d9534ee..2f5b07e 100644 >> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c >> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c >> @@ -232,6 +232,7 @@ static void ibmvscsi_task(void *data) >>> while ((crq = crq_queue_next_crq(&hostdata->queue)) != NULL) { >>> ibmvscsi_handle_crq(crq, hostdata); >>> crq->valid = VIOSRP_CRQ_FREE; >>> + wmb(); >>> } >> >>> vio_enable_interrupts(vdev); >> @@ -240,6 +241,7 @@ static void ibmvscsi_task(void *data) >>> vio_disable_interrupts(vdev); >>> ibmvscsi_handle_crq(crq, hostdata); >>> crq->valid = VIOSRP_CRQ_FREE; >>> + wmb(); >>> } else { >>> done = 1; >>> } > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c index d9534ee..2f5b07e 100644 --- a/drivers/scsi/ibmvscsi/ibmvscsi.c +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c @@ -232,6 +232,7 @@ static void ibmvscsi_task(void *data) while ((crq = crq_queue_next_crq(&hostdata->queue)) != NULL) { ibmvscsi_handle_crq(crq, hostdata); crq->valid = VIOSRP_CRQ_FREE; + wmb(); } vio_enable_interrupts(vdev); @@ -240,6 +241,7 @@ static void ibmvscsi_task(void *data) vio_disable_interrupts(vdev); ibmvscsi_handle_crq(crq, hostdata); crq->valid = VIOSRP_CRQ_FREE; + wmb(); } else { done = 1; }
The first byte of each CRQ entry is used to indicate whether an entry is a valid response or free for the VIOS to use. After processing a response the driver sets the valid byte to zero to indicate the entry is now free to be reused. Add a memory barrier after this write to ensure no other stores are reordered when updating the valid byte. Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com> --- drivers/scsi/ibmvscsi/ibmvscsi.c | 2 ++ 1 file changed, 2 insertions(+)