* [BUG] bug when enabling VM DEBUG @ 2010-05-10 13:06 saeed bishara 2010-05-10 13:09 ` saeed bishara 2010-05-11 15:27 ` saeed bishara 0 siblings, 2 replies; 11+ messages in thread From: saeed bishara @ 2010-05-10 13:06 UTC (permalink / raw) To: IDE/ATA development list Hi, I got the following bug when enabling the VM_DEBGU on 2.6.34-rc6: Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = c0004000 [00000000] *pgd=00000000 Internal error: Oops: 805 [#1] last sysfs file: Modules linked in: CPU: 0 Not tainted (2.6.34-rc6-00176-g00047b8-dirty #331) PC is at flush_dcache_page+0x2c/0xc4 LR is at ata_pio_sector+0xd8/0x124 pc : [<c002d30c>] lr : [<c01c29d4>] psr: 20000013 sp : f5513ee8 ip : 79a50000 fp : 00001ee8 r10: 00000058 r9 : f54f0000 r8 : 00000000 r7 : f54f1358 r6 : c03e6b84 r5 : 00000000 r4 : 00000000 r3 : 00000000 r2 : 00000000 r1 : f54f17b8 r0 : c0ac4e20 Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel Control: 10c5387d Table: 00004019 DAC: 00000017 Process ata/0 (pid: 145, stack limit = 0xf55122f0) Stack: (0xf5513ee8 to 0xf5514000) 3ee0: c0ac4e20 c01c29d4 00000009 f54f1358 f54f1358 00000001 3f00: c040d18c c01c2ad8 00000005 00000009 f54f0000 c01c3204 c03cfe78 00000000 3f20: 00200200 f54f13d4 c00446e4 00000001 f54f1424 00000000 00000000 00000000 3f40: 00000000 00000000 ffffffff 00000009 f54f0000 f54f1358 00000001 c040d18c 3f60: 00000005 0000000a 00001ee8 c01c46d8 f5413c70 f54b5b68 c01c45d4 f54b5b60 3f80: f5512000 f5513f9c 00000000 00000000 00000000 c004b2c0 00000000 00000000 3fa0: f54be0c0 c004e12c f5513fa8 f5513fa8 f5429ec8 f5513fd4 f5429ec8 f54b5b60 3fc0: c004b1c8 00000000 00000000 c004e014 00000000 00000000 f5513fd8 f5513fd8 3fe0: 00000000 00000000 00000000 00000000 00000000 c0026dec bc7e4afb 6a3a8a30 [<c002d30c>] (flush_dcache_page+0x2c/0xc4) from [<c01c29d4>] (ata_pio_sector+0xd8/0x124) [<c01c29d4>] (ata_pio_sector+0xd8/0x124) from [<c01c2ad8>] (ata_pio_sectors+0xb8/0xc4) [<c01c2ad8>] (ata_pio_sectors+0xb8/0xc4) from [<c01c3204>] (ata_sff_hsm_move+0x618/0x76c) [<c01c3204>] (ata_sff_hsm_move+0x618/0x76c) from [<c01c46d8>] (ata_pio_task+0x104/0x124) [<c01c46d8>] (ata_pio_task+0x104/0x124) from [<c004b2c0>] (worker_thread+0xf8/0x170) [<c004b2c0>] (worker_thread+0xf8/0x170) from [<c004e014>] (kthread+0x78/0x80) [<c004e014>] (kthread+0x78/0x80) from [<c0026dec>] (kernel_thread_exit+0x0/0x8) Code: e5904010 e3130080 0a000002 e3a03000 (e5833000) ---[ end trace 8a7ce1ae66250005 ]--- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] bug when enabling VM DEBUG 2010-05-10 13:06 [BUG] bug when enabling VM DEBUG saeed bishara @ 2010-05-10 13:09 ` saeed bishara 2010-05-11 15:27 ` saeed bishara 1 sibling, 0 replies; 11+ messages in thread From: saeed bishara @ 2010-05-10 13:09 UTC (permalink / raw) To: IDE/ATA development list forgot to mention that the system is arm based. On Mon, May 10, 2010 at 4:06 PM, saeed bishara <saeed.bishara@gmail.com> wrote: > Hi, > I got the following bug when enabling the VM_DEBGU on 2.6.34-rc6: > > Unable to handle kernel NULL pointer dereference at virtual address 00000000 > pgd = c0004000 > [00000000] *pgd=00000000 > Internal error: Oops: 805 [#1] > last sysfs file: > Modules linked in: > CPU: 0 Not tainted (2.6.34-rc6-00176-g00047b8-dirty #331) > PC is at flush_dcache_page+0x2c/0xc4 > LR is at ata_pio_sector+0xd8/0x124 > pc : [<c002d30c>] lr : [<c01c29d4>] psr: 20000013 > sp : f5513ee8 ip : 79a50000 fp : 00001ee8 > r10: 00000058 r9 : f54f0000 r8 : 00000000 > r7 : f54f1358 r6 : c03e6b84 r5 : 00000000 r4 : 00000000 > r3 : 00000000 r2 : 00000000 r1 : f54f17b8 r0 : c0ac4e20 > Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel > Control: 10c5387d Table: 00004019 DAC: 00000017 > Process ata/0 (pid: 145, stack limit = 0xf55122f0) > Stack: (0xf5513ee8 to 0xf5514000) > 3ee0: c0ac4e20 c01c29d4 00000009 f54f1358 f54f1358 00000001 > 3f00: c040d18c c01c2ad8 00000005 00000009 f54f0000 c01c3204 c03cfe78 00000000 > 3f20: 00200200 f54f13d4 c00446e4 00000001 f54f1424 00000000 00000000 00000000 > 3f40: 00000000 00000000 ffffffff 00000009 f54f0000 f54f1358 00000001 c040d18c > 3f60: 00000005 0000000a 00001ee8 c01c46d8 f5413c70 f54b5b68 c01c45d4 f54b5b60 > 3f80: f5512000 f5513f9c 00000000 00000000 00000000 c004b2c0 00000000 00000000 > 3fa0: f54be0c0 c004e12c f5513fa8 f5513fa8 f5429ec8 f5513fd4 f5429ec8 f54b5b60 > 3fc0: c004b1c8 00000000 00000000 c004e014 00000000 00000000 f5513fd8 f5513fd8 > 3fe0: 00000000 00000000 00000000 00000000 00000000 c0026dec bc7e4afb 6a3a8a30 > [<c002d30c>] (flush_dcache_page+0x2c/0xc4) from [<c01c29d4>] > (ata_pio_sector+0xd8/0x124) > [<c01c29d4>] (ata_pio_sector+0xd8/0x124) from [<c01c2ad8>] > (ata_pio_sectors+0xb8/0xc4) > [<c01c2ad8>] (ata_pio_sectors+0xb8/0xc4) from [<c01c3204>] > (ata_sff_hsm_move+0x618/0x76c) > [<c01c3204>] (ata_sff_hsm_move+0x618/0x76c) from [<c01c46d8>] > (ata_pio_task+0x104/0x124) > [<c01c46d8>] (ata_pio_task+0x104/0x124) from [<c004b2c0>] > (worker_thread+0xf8/0x170) > [<c004b2c0>] (worker_thread+0xf8/0x170) from [<c004e014>] (kthread+0x78/0x80) > [<c004e014>] (kthread+0x78/0x80) from [<c0026dec>] (kernel_thread_exit+0x0/0x8) > Code: e5904010 e3130080 0a000002 e3a03000 (e5833000) > ---[ end trace 8a7ce1ae66250005 ]--- > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] bug when enabling VM DEBUG 2010-05-10 13:06 [BUG] bug when enabling VM DEBUG saeed bishara 2010-05-10 13:09 ` saeed bishara @ 2010-05-11 15:27 ` saeed bishara 2010-05-12 2:41 ` Robert Hancock 1 sibling, 1 reply; 11+ messages in thread From: saeed bishara @ 2010-05-11 15:27 UTC (permalink / raw) To: Jeff Garzik; +Cc: DE/ATA development list, lkml, linux-arm-kernel Jeff, seems that the libata do pio to buffer which was allocated with kmalloc, and under arm arch, the flush_dcache_page() is needed to prevent cache aliasing, the later function calls the page_mapping which falls on PageSlab(page) as the flush_dcache_page() is needed to prevent aliasing, it can be skipped if the page is Slab as such pages are used only by the kernel. here a suggested patch: --- a/drivers/ata/libata-sff.c +++ b/drivers/ata/libata-sff.c @@ -894,7 +894,7 @@ static void ata_pio_sector(struct ata_queued_cmd *qc) do_write); } - if (!do_write) + if (!do_write && !PageSlab(page)) flush_dcache_page(page); saeed On Mon, May 10, 2010 at 4:06 PM, saeed bishara <saeed.bishara@gmail.com> wrote: > Hi, > I got the following bug when enabling the VM_DEBGU on 2.6.34-rc6: > > Unable to handle kernel NULL pointer dereference at virtual address 00000000 > pgd = c0004000 > [00000000] *pgd=00000000 > Internal error: Oops: 805 [#1] > last sysfs file: > Modules linked in: > CPU: 0 Not tainted (2.6.34-rc6-00176-g00047b8-dirty #331) > PC is at flush_dcache_page+0x2c/0xc4 > LR is at ata_pio_sector+0xd8/0x124 > pc : [<c002d30c>] lr : [<c01c29d4>] psr: 20000013 > sp : f5513ee8 ip : 79a50000 fp : 00001ee8 > r10: 00000058 r9 : f54f0000 r8 : 00000000 > r7 : f54f1358 r6 : c03e6b84 r5 : 00000000 r4 : 00000000 > r3 : 00000000 r2 : 00000000 r1 : f54f17b8 r0 : c0ac4e20 > Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel > Control: 10c5387d Table: 00004019 DAC: 00000017 > Process ata/0 (pid: 145, stack limit = 0xf55122f0) > Stack: (0xf5513ee8 to 0xf5514000) > 3ee0: c0ac4e20 c01c29d4 00000009 f54f1358 f54f1358 00000001 > 3f00: c040d18c c01c2ad8 00000005 00000009 f54f0000 c01c3204 c03cfe78 00000000 > 3f20: 00200200 f54f13d4 c00446e4 00000001 f54f1424 00000000 00000000 00000000 > 3f40: 00000000 00000000 ffffffff 00000009 f54f0000 f54f1358 00000001 c040d18c > 3f60: 00000005 0000000a 00001ee8 c01c46d8 f5413c70 f54b5b68 c01c45d4 f54b5b60 > 3f80: f5512000 f5513f9c 00000000 00000000 00000000 c004b2c0 00000000 00000000 > 3fa0: f54be0c0 c004e12c f5513fa8 f5513fa8 f5429ec8 f5513fd4 f5429ec8 f54b5b60 > 3fc0: c004b1c8 00000000 00000000 c004e014 00000000 00000000 f5513fd8 f5513fd8 > 3fe0: 00000000 00000000 00000000 00000000 00000000 c0026dec bc7e4afb 6a3a8a30 > [<c002d30c>] (flush_dcache_page+0x2c/0xc4) from [<c01c29d4>] > (ata_pio_sector+0xd8/0x124) > [<c01c29d4>] (ata_pio_sector+0xd8/0x124) from [<c01c2ad8>] > (ata_pio_sectors+0xb8/0xc4) > [<c01c2ad8>] (ata_pio_sectors+0xb8/0xc4) from [<c01c3204>] > (ata_sff_hsm_move+0x618/0x76c) > [<c01c3204>] (ata_sff_hsm_move+0x618/0x76c) from [<c01c46d8>] > (ata_pio_task+0x104/0x124) > [<c01c46d8>] (ata_pio_task+0x104/0x124) from [<c004b2c0>] > (worker_thread+0xf8/0x170) > [<c004b2c0>] (worker_thread+0xf8/0x170) from [<c004e014>] (kthread+0x78/0x80) > [<c004e014>] (kthread+0x78/0x80) from [<c0026dec>] (kernel_thread_exit+0x0/0x8) > Code: e5904010 e3130080 0a000002 e3a03000 (e5833000) > ---[ end trace 8a7ce1ae66250005 ]--- > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] bug when enabling VM DEBUG 2010-05-11 15:27 ` saeed bishara @ 2010-05-12 2:41 ` Robert Hancock 2010-05-12 11:10 ` Catalin Marinas 0 siblings, 1 reply; 11+ messages in thread From: Robert Hancock @ 2010-05-12 2:41 UTC (permalink / raw) To: saeed bishara Cc: Jeff Garzik, DE/ATA development list, lkml, linux-arm-kernel On 05/11/2010 09:27 AM, saeed bishara wrote: > Jeff, > seems that the libata do pio to buffer which was allocated with > kmalloc, and under arm arch, the flush_dcache_page() is needed to > prevent cache aliasing, the later function calls the page_mapping > which falls on PageSlab(page) > > as the flush_dcache_page() is needed to prevent aliasing, it can be > skipped if the page is Slab as such pages are used only by the kernel. > here a suggested patch: > > --- a/drivers/ata/libata-sff.c > +++ b/drivers/ata/libata-sff.c > @@ -894,7 +894,7 @@ static void ata_pio_sector(struct ata_queued_cmd *qc) > do_write); > } > > - if (!do_write) > + if (!do_write&& !PageSlab(page)) > flush_dcache_page(page); > > saeed I would think that check belongs inside flush_dcache_page itself, rather than forcing every driver to include it.. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] bug when enabling VM DEBUG 2010-05-12 2:41 ` Robert Hancock @ 2010-05-12 11:10 ` Catalin Marinas 2010-05-12 11:53 ` Sebastian Andrzej Siewior 0 siblings, 1 reply; 11+ messages in thread From: Catalin Marinas @ 2010-05-12 11:10 UTC (permalink / raw) To: Robert Hancock Cc: saeed bishara, linux-arm-kernel, DE/ATA development list, lkml, Jeff Garzik, Sebastian Andrzej Siewior On Wed, 2010-05-12 at 03:41 +0100, Robert Hancock wrote: > On 05/11/2010 09:27 AM, saeed bishara wrote: > > Jeff, > > seems that the libata do pio to buffer which was allocated with > > kmalloc, and under arm arch, the flush_dcache_page() is needed to > > prevent cache aliasing, the later function calls the page_mapping > > which falls on PageSlab(page) > > > > as the flush_dcache_page() is needed to prevent aliasing, it can be > > skipped if the page is Slab as such pages are used only by the kernel. > > here a suggested patch: > > > > --- a/drivers/ata/libata-sff.c > > +++ b/drivers/ata/libata-sff.c > > @@ -894,7 +894,7 @@ static void ata_pio_sector(struct ata_queued_cmd *qc) > > do_write); > > } > > > > - if (!do_write) > > + if (!do_write&& !PageSlab(page)) > > flush_dcache_page(page); > > I would think that check belongs inside flush_dcache_page itself, rather > than forcing every driver to include it.. Sebastian (cc'ed) reported this as well for MIPS. I think it makes sense for this check to be done in the flush_dcache_page() function. -- Catalin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] bug when enabling VM DEBUG 2010-05-12 11:10 ` Catalin Marinas @ 2010-05-12 11:53 ` Sebastian Andrzej Siewior 2010-05-12 12:24 ` Catalin Marinas 0 siblings, 1 reply; 11+ messages in thread From: Sebastian Andrzej Siewior @ 2010-05-12 11:53 UTC (permalink / raw) To: Catalin Marinas Cc: Robert Hancock, saeed bishara, linux-arm-kernel, DE/ATA development list, lkml, Jeff Garzik, Sebastian Andrzej Siewior * Catalin Marinas | 2010-05-12 12:10:39 [+0100]: >> > --- a/drivers/ata/libata-sff.c >> > +++ b/drivers/ata/libata-sff.c >> > @@ -894,7 +894,7 @@ static void ata_pio_sector(struct ata_queued_cmd *qc) >> > do_write); >> > } >> > >> > - if (!do_write) >> > + if (!do_write&& !PageSlab(page)) >> > flush_dcache_page(page); >> >> I would think that check belongs inside flush_dcache_page itself, rather >> than forcing every driver to include it.. > >Sebastian (cc'ed) reported this as well for MIPS. Thx. The patch above looks what I've sent a while ago. Jeff was going to merge it afaik. >I think it makes sense for this check to be done in the >flush_dcache_page() function. Why should flush_dcache_page() not flush pages you tell it? >From Documentation/cachetlb.txt: | NOTE: This routine need only be called for page cache pages | which can potentially ever be mapped into the address | space of a user process. So for example, VFS layer code | handling vfs symlinks in the page cache need not call | this interface at all. A page from slab or stack is not going to see the sky of user land and therefore it should not be fed into flush_dcache_page(). Sebastian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] bug when enabling VM DEBUG 2010-05-12 11:53 ` Sebastian Andrzej Siewior @ 2010-05-12 12:24 ` Catalin Marinas 2010-05-12 13:06 ` Jamie Lokier 0 siblings, 1 reply; 11+ messages in thread From: Catalin Marinas @ 2010-05-12 12:24 UTC (permalink / raw) To: Sebastian Andrzej Siewior Cc: Robert Hancock, saeed bishara, linux-arm-kernel, DE/ATA development list, lkml, Jeff Garzik On Wed, 2010-05-12 at 12:53 +0100, Sebastian Andrzej Siewior wrote: > * Catalin Marinas | 2010-05-12 12:10:39 [+0100]: > > >> > --- a/drivers/ata/libata-sff.c > >> > +++ b/drivers/ata/libata-sff.c > >> > @@ -894,7 +894,7 @@ static void ata_pio_sector(struct ata_queued_cmd *qc) > >> > do_write); > >> > } > >> > > >> > - if (!do_write) > >> > + if (!do_write&& !PageSlab(page)) > >> > flush_dcache_page(page); > >> > >> I would think that check belongs inside flush_dcache_page itself, rather > >> than forcing every driver to include it.. > > > >Sebastian (cc'ed) reported this as well for MIPS. > Thx. The patch above looks what I've sent a while ago. Jeff was going to > merge it afaik. > > >I think it makes sense for this check to be done in the > >flush_dcache_page() function. > > Why should flush_dcache_page() not flush pages you tell it? > From Documentation/cachetlb.txt: > | NOTE: This routine need only be called for page cache pages > | which can potentially ever be mapped into the address > | space of a user process. So for example, VFS layer code > | handling vfs symlinks in the page cache need not call > | this interface at all. > > A page from slab or stack is not going to see the sky of user land and > therefore it should not be fed into flush_dcache_page(). You are right :), so fixing the driver is the best approach. -- Catalin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] bug when enabling VM DEBUG 2010-05-12 12:24 ` Catalin Marinas @ 2010-05-12 13:06 ` Jamie Lokier 2010-05-12 23:22 ` Robert Hancock 2010-05-13 11:30 ` Catalin Marinas 0 siblings, 2 replies; 11+ messages in thread From: Jamie Lokier @ 2010-05-12 13:06 UTC (permalink / raw) To: Catalin Marinas Cc: Sebastian Andrzej Siewior, Jeff Garzik, lkml, DE/ATA development list, saeed bishara, linux-arm-kernel, Robert Hancock Catalin Marinas wrote: > On Wed, 2010-05-12 at 12:53 +0100, Sebastian Andrzej Siewior wrote: > > * Catalin Marinas | 2010-05-12 12:10:39 [+0100]: > > > > >> > --- a/drivers/ata/libata-sff.c > > >> > +++ b/drivers/ata/libata-sff.c > > >> > @@ -894,7 +894,7 @@ static void ata_pio_sector(struct ata_queued_cmd *qc) > > >> > do_write); > > >> > } > > >> > > > >> > - if (!do_write) > > >> > + if (!do_write&& !PageSlab(page)) > > >> > flush_dcache_page(page); > > >> > > >> I would think that check belongs inside flush_dcache_page itself, rather > > >> than forcing every driver to include it.. > > > > > >Sebastian (cc'ed) reported this as well for MIPS. > > Thx. The patch above looks what I've sent a while ago. Jeff was going to > > merge it afaik. > > > > >I think it makes sense for this check to be done in the > > >flush_dcache_page() function. > > > > Why should flush_dcache_page() not flush pages you tell it? > > From Documentation/cachetlb.txt: > > | NOTE: This routine need only be called for page cache pages > > | which can potentially ever be mapped into the address > > | space of a user process. So for example, VFS layer code > > | handling vfs symlinks in the page cache need not call > > | this interface at all. > > > > A page from slab or stack is not going to see the sky of user land and > > therefore it should not be fed into flush_dcache_page(). > > You are right :), so fixing the driver is the best approach. It worries me that a driver has any knowledge of the PageSlab() flag, though. Especially uncommented knowledge. That flag seems VM internal, and it's conceptually iffy: Kernel code using get_free_pages() and using that for I/O also does not see the sky of user land. If all the PIO drivers have to be changed, I'd be happier with: flush_dcache_page_for_pio() which wraps the check, explains it, and provides a single place to change if needed. -- Jamie ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] bug when enabling VM DEBUG 2010-05-12 13:06 ` Jamie Lokier @ 2010-05-12 23:22 ` Robert Hancock 2010-05-13 11:30 ` Catalin Marinas 1 sibling, 0 replies; 11+ messages in thread From: Robert Hancock @ 2010-05-12 23:22 UTC (permalink / raw) To: Jamie Lokier Cc: Catalin Marinas, Sebastian Andrzej Siewior, Jeff Garzik, lkml, DE/ATA development list, saeed bishara, linux-arm-kernel On Wed, May 12, 2010 at 7:06 AM, Jamie Lokier <jamie@shareable.org> wrote: > Catalin Marinas wrote: >> On Wed, 2010-05-12 at 12:53 +0100, Sebastian Andrzej Siewior wrote: >> > * Catalin Marinas | 2010-05-12 12:10:39 [+0100]: >> > >> > >> > --- a/drivers/ata/libata-sff.c >> > >> > +++ b/drivers/ata/libata-sff.c >> > >> > @@ -894,7 +894,7 @@ static void ata_pio_sector(struct ata_queued_cmd *qc) >> > >> > do_write); >> > >> > } >> > >> > >> > >> > - if (!do_write) >> > >> > + if (!do_write&& !PageSlab(page)) >> > >> > flush_dcache_page(page); >> > >> >> > >> I would think that check belongs inside flush_dcache_page itself, rather >> > >> than forcing every driver to include it.. >> > > >> > >Sebastian (cc'ed) reported this as well for MIPS. >> > Thx. The patch above looks what I've sent a while ago. Jeff was going to >> > merge it afaik. >> > >> > >I think it makes sense for this check to be done in the >> > >flush_dcache_page() function. >> > >> > Why should flush_dcache_page() not flush pages you tell it? >> > From Documentation/cachetlb.txt: >> > | NOTE: This routine need only be called for page cache pages >> > | which can potentially ever be mapped into the address >> > | space of a user process. So for example, VFS layer code >> > | handling vfs symlinks in the page cache need not call >> > | this interface at all. >> > >> > A page from slab or stack is not going to see the sky of user land and >> > therefore it should not be fed into flush_dcache_page(). >> >> You are right :), so fixing the driver is the best approach. > > It worries me that a driver has any knowledge of the PageSlab() flag, > though. Especially uncommented knowledge. That flag seems VM > internal, and it's conceptually iffy: Kernel code using > get_free_pages() and using that for I/O also does not see the sky of > user land. > > If all the PIO drivers have to be changed, I'd be happier with: > > flush_dcache_page_for_pio() > > which wraps the check, explains it, and provides a single place to > change if needed. Indeed.. It's obscure enough for drivers to need to worry about these details of cache flushing/invalidation at all, let alone for them to have to worry about even more obscure details.. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] bug when enabling VM DEBUG 2010-05-12 13:06 ` Jamie Lokier 2010-05-12 23:22 ` Robert Hancock @ 2010-05-13 11:30 ` Catalin Marinas 2010-05-14 21:48 ` Jeff Garzik 1 sibling, 1 reply; 11+ messages in thread From: Catalin Marinas @ 2010-05-13 11:30 UTC (permalink / raw) To: Jamie Lokier Cc: Sebastian Andrzej Siewior, Jeff Garzik, lkml, DE/ATA development list, saeed bishara, linux-arm-kernel, Robert Hancock On Wed, 2010-05-12 at 14:06 +0100, Jamie Lokier wrote: > Catalin Marinas wrote: > > On Wed, 2010-05-12 at 12:53 +0100, Sebastian Andrzej Siewior wrote: > > > * Catalin Marinas | 2010-05-12 12:10:39 [+0100]: > > > > > > >> > --- a/drivers/ata/libata-sff.c > > > >> > +++ b/drivers/ata/libata-sff.c > > > >> > @@ -894,7 +894,7 @@ static void ata_pio_sector(struct ata_queued_cmd *qc) > > > >> > do_write); > > > >> > } > > > >> > > > > >> > - if (!do_write) > > > >> > + if (!do_write&& !PageSlab(page)) > > > >> > flush_dcache_page(page); > > > >> > > > >> I would think that check belongs inside flush_dcache_page itself, rather > > > >> than forcing every driver to include it.. > > > > > > > >Sebastian (cc'ed) reported this as well for MIPS. > > > Thx. The patch above looks what I've sent a while ago. Jeff was going to > > > merge it afaik. > > > > > > >I think it makes sense for this check to be done in the > > > >flush_dcache_page() function. > > > > > > Why should flush_dcache_page() not flush pages you tell it? > > > From Documentation/cachetlb.txt: > > > | NOTE: This routine need only be called for page cache pages > > > | which can potentially ever be mapped into the address > > > | space of a user process. So for example, VFS layer code > > > | handling vfs symlinks in the page cache need not call > > > | this interface at all. > > > > > > A page from slab or stack is not going to see the sky of user land and > > > therefore it should not be fed into flush_dcache_page(). > > > > You are right :), so fixing the driver is the best approach. > > It worries me that a driver has any knowledge of the PageSlab() flag, > though. Especially uncommented knowledge. That flag seems VM > internal, and it's conceptually iffy: Kernel code using > get_free_pages() and using that for I/O also does not see the sky of > user land. > > If all the PIO drivers have to be changed, I'd be happier with: > > flush_dcache_page_for_pio() > > which wraps the check, explains it, and provides a single place to > change if needed. I already proposed a PIO cache flushing API on linux-arch. I tried to revive the discussion with another patch for cachetlb.txt but it hasn't drawn much interest. On ARM we are trying to change the I/D cache handling similar to IA-64 and PowerPC (PG_arch_1 would mean "clean" rather than "dirty"). With this change, my original patch to libata-sff.c can be dropped (well, other architectures may still be broken). -- Catalin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] bug when enabling VM DEBUG 2010-05-13 11:30 ` Catalin Marinas @ 2010-05-14 21:48 ` Jeff Garzik 0 siblings, 0 replies; 11+ messages in thread From: Jeff Garzik @ 2010-05-14 21:48 UTC (permalink / raw) To: Catalin Marinas Cc: Jamie Lokier, Sebastian Andrzej Siewior, lkml, DE/ATA development list, saeed bishara, linux-arm-kernel, Robert Hancock On 05/13/2010 07:30 AM, Catalin Marinas wrote: > I already proposed a PIO cache flushing API on linux-arch. I tried to > revive the discussion with another patch for cachetlb.txt but it hasn't > drawn much interest. Oh well. I'll apply the local-to-libata patch, and hope that someone else proposes a better API. Jeff ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-05-14 21:48 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-05-10 13:06 [BUG] bug when enabling VM DEBUG saeed bishara 2010-05-10 13:09 ` saeed bishara 2010-05-11 15:27 ` saeed bishara 2010-05-12 2:41 ` Robert Hancock 2010-05-12 11:10 ` Catalin Marinas 2010-05-12 11:53 ` Sebastian Andrzej Siewior 2010-05-12 12:24 ` Catalin Marinas 2010-05-12 13:06 ` Jamie Lokier 2010-05-12 23:22 ` Robert Hancock 2010-05-13 11:30 ` Catalin Marinas 2010-05-14 21:48 ` Jeff Garzik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).