Patchwork [2/2] libata: disable preemption during PIO copy

login
register
mail settings
Submitter Sebastian Siewior
Date March 21, 2010, 9:53 p.m.
Message ID <20100321215308.GB27709@Chamillionaire.breakpoint.cc>
Download mbox | patch
Permalink /patch/48239/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Sebastian Siewior - March 21, 2010, 9:53 p.m.
If the request has been made in user context it could be moved to a
FUJITA Tomonori - March 23, 2010, 9:49 a.m.
On Sun, 21 Mar 2010 22:53:08 +0100
Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:

> If the request has been made in user context it could be moved to a
> different CPU on a SMP machine between the copy and cache flush. Thus we
> could flush the dcache on the wrong CPU.

This issue should be addressed in flush_dcache_page() inside?


> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> ---
>  drivers/ata/libata-sff.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 83ecf48..dca9f90 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -874,6 +874,9 @@ static void ata_pio_sector(struct ata_queued_cmd *qc)
>  
>  	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
>  
> +#ifdef CONFIG_SMP
> +	preempt_disable();
> +#endif
>  	if (PageHighMem(page)) {
>  		unsigned long flags;
>  
> @@ -896,6 +899,9 @@ static void ata_pio_sector(struct ata_queued_cmd *qc)
>  	if (!do_write && !PageSlab(page))
>  		flush_dcache_page(page);
>  
> +#ifdef CONFIG_SMP
> +	preempt_enable();
> +#endif
>  	qc->curbytes += qc->sect_size;
>  	qc->cursg_ofs += qc->sect_size;
>  
> -- 
> 1.6.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Siewior - March 23, 2010, 11:01 a.m.
* FUJITA Tomonori | 2010-03-23 18:49:04 [+0900]:

>On Sun, 21 Mar 2010 22:53:08 +0100
>Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:
>
>> If the request has been made in user context it could be moved to a
>> different CPU on a SMP machine between the copy and cache flush. Thus we
>> could flush the dcache on the wrong CPU.
>
>This issue should be addressed in flush_dcache_page() inside?

How so? You have to remain on the same CPU on which you started the copy
process. Once you get to flush_dcache_page() you may have allready
switched the CPU.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
FUJITA Tomonori - March 23, 2010, 11:51 a.m.
On Tue, 23 Mar 2010 12:01:24 +0100
Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:

> * FUJITA Tomonori | 2010-03-23 18:49:04 [+0900]:
> 
> >On Sun, 21 Mar 2010 22:53:08 +0100
> >Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:
> >
> >> If the request has been made in user context it could be moved to a
> >> different CPU on a SMP machine between the copy and cache flush. Thus we
> >> could flush the dcache on the wrong CPU.
> >
> >This issue should be addressed in flush_dcache_page() inside?
> 
> How so? You have to remain on the same CPU on which you started the copy
> process. Once you get to flush_dcache_page() you may have allready
> switched the CPU.

Oops, I thought that you trying to address on the deferred
flush_dcache_page() issue. I don't know enough about how this pio code
works, but if your description is correct, then the patch is fine, I
guess.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Siewior - March 24, 2010, 9:52 p.m.
* FUJITA Tomonori | 2010-03-23 20:51:34 [+0900]:

>> How so? You have to remain on the same CPU on which you started the copy
>> process. Once you get to flush_dcache_page() you may have allready
>> switched the CPU.
>
>Oops, I thought that you trying to address on the deferred
>flush_dcache_page() issue. I don't know enough about how this pio code
>works, but if your description is correct, then the patch is fine, I
>guess.

No, it is not. The dcache flush is a global one so a CPU switch does not
matter. So I NAK that one myself.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

different CPU on a SMP machine between the copy and cache flush. Thus we
could flush the dcache on the wrong CPU.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 drivers/ata/libata-sff.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 83ecf48..dca9f90 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -874,6 +874,9 @@  static void ata_pio_sector(struct ata_queued_cmd *qc)
 
 	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
 
+#ifdef CONFIG_SMP
+	preempt_disable();
+#endif
 	if (PageHighMem(page)) {
 		unsigned long flags;
 
@@ -896,6 +899,9 @@  static void ata_pio_sector(struct ata_queued_cmd *qc)
 	if (!do_write && !PageSlab(page))
 		flush_dcache_page(page);
 
+#ifdef CONFIG_SMP
+	preempt_enable();
+#endif
 	qc->curbytes += qc->sect_size;
 	qc->cursg_ofs += qc->sect_size;