| Message ID | 20220405194747.2386619-6-jane.chu@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | DAX poison recovery | expand |
On Tue, Apr 05, 2022 at 01:47:46PM -0600, Jane Chu wrote: > + pmem_clear_bb(pmem, to_sect(pmem, offset), cleared >> SECTOR_SHIFT); > + return (cleared < len) ? BLK_STS_IOERR : BLK_STS_OK; No need for the braces. That being said perosnally I find a simple: if (cleared < len) return BLK_STS_IOERR; return BLK_STS_OK; much easier to read anyway. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On 4/5/2022 10:04 PM, Christoph Hellwig wrote: > On Tue, Apr 05, 2022 at 01:47:46PM -0600, Jane Chu wrote: >> + pmem_clear_bb(pmem, to_sect(pmem, offset), cleared >> SECTOR_SHIFT); >> + return (cleared < len) ? BLK_STS_IOERR : BLK_STS_OK; > > No need for the braces. That being said perosnally I find a simple: > > if (cleared < len) > return BLK_STS_IOERR; > return BLK_STS_OK; > > much easier to read anyway. will do. > > Otherwise looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> Thanks! -jane
On Tue, Apr 5, 2022 at 12:48 PM Jane Chu <jane.chu@oracle.com> wrote: > > Refactor the pmem_clear_poison() in order to share common code > later. > I would just add a note here about why, i.e. to factor out the common shared code between the typical write path and the recovery write path. > Signed-off-by: Jane Chu <jane.chu@oracle.com> > --- > drivers/nvdimm/pmem.c | 78 ++++++++++++++++++++++++++++--------------- > 1 file changed, 52 insertions(+), 26 deletions(-) > > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c > index 0400c5a7ba39..56596be70400 100644 > --- a/drivers/nvdimm/pmem.c > +++ b/drivers/nvdimm/pmem.c > @@ -45,10 +45,27 @@ static struct nd_region *to_region(struct pmem_device *pmem) > return to_nd_region(to_dev(pmem)->parent); > } > > -static void hwpoison_clear(struct pmem_device *pmem, > - phys_addr_t phys, unsigned int len) > +static phys_addr_t to_phys(struct pmem_device *pmem, phys_addr_t offset) > { > + return (pmem->phys_addr + offset); Christoph already mentioned dropping the unnecessary parenthesis. > +} > + > +static sector_t to_sect(struct pmem_device *pmem, phys_addr_t offset) > +{ > + return (offset - pmem->data_offset) >> SECTOR_SHIFT; > +} > + > +static phys_addr_t to_offset(struct pmem_device *pmem, sector_t sector) > +{ > + return ((sector << SECTOR_SHIFT) + pmem->data_offset); > +} > + > +static void pmem_clear_hwpoison(struct pmem_device *pmem, phys_addr_t offset, > + unsigned int len) Perhaps now is a good time to rename this to something else like pmem_clear_mce_nospec()? Just to make it more distinct from pmem_clear_poison(). While "hwpoison" is the page flag name pmem_clear_poison() is the function that's actually clearing the poison in hardware ("hw") and the new pmem_clear_mce_nospec() is toggling the page back into service. > +{ > + phys_addr_t phys = to_phys(pmem, offset); > unsigned long pfn_start, pfn_end, pfn; > + unsigned int blks = len >> SECTOR_SHIFT; > > /* only pmem in the linear map supports HWPoison */ > if (is_vmalloc_addr(pmem->virt_addr)) > @@ -67,35 +84,44 @@ static void hwpoison_clear(struct pmem_device *pmem, > if (test_and_clear_pmem_poison(page)) > clear_mce_nospec(pfn); > } > + > + dev_dbg(to_dev(pmem), "%#llx clear %u sector%s\n", > + (unsigned long long) to_sect(pmem, offset), blks, > + blks > 1 ? "s" : ""); In anticipation of better tracing support and the fact that this is no longer called from pmem_clear_poison() let's drop it for now. > } > > -static blk_status_t pmem_clear_poison(struct pmem_device *pmem, > +static void pmem_clear_bb(struct pmem_device *pmem, sector_t sector, long blks) > +{ > + if (blks == 0) > + return; > + badblocks_clear(&pmem->bb, sector, blks); > + if (pmem->bb_state) > + sysfs_notify_dirent(pmem->bb_state); > +} > + > +static long __pmem_clear_poison(struct pmem_device *pmem, > phys_addr_t offset, unsigned int len) > { > - struct device *dev = to_dev(pmem); > - sector_t sector; > - long cleared; > - blk_status_t rc = BLK_STS_OK; > - > - sector = (offset - pmem->data_offset) / 512; > - > - cleared = nvdimm_clear_poison(dev, pmem->phys_addr + offset, len); > - if (cleared < len) > - rc = BLK_STS_IOERR; > - if (cleared > 0 && cleared / 512) { > - hwpoison_clear(pmem, pmem->phys_addr + offset, cleared); > - cleared /= 512; > - dev_dbg(dev, "%#llx clear %ld sector%s\n", > - (unsigned long long) sector, cleared, > - cleared > 1 ? "s" : ""); > - badblocks_clear(&pmem->bb, sector, cleared); > - if (pmem->bb_state) > - sysfs_notify_dirent(pmem->bb_state); > + phys_addr_t phys = to_phys(pmem, offset); > + long cleared = nvdimm_clear_poison(to_dev(pmem), phys, len); > + > + if (cleared > 0) { > + pmem_clear_hwpoison(pmem, offset, cleared); > + arch_invalidate_pmem(pmem->virt_addr + offset, len); > } > + return cleared; > +} > > - arch_invalidate_pmem(pmem->virt_addr + offset, len); > +static blk_status_t pmem_clear_poison(struct pmem_device *pmem, > + phys_addr_t offset, unsigned int len) > +{ > + long cleared = __pmem_clear_poison(pmem, offset, len); > > - return rc; > + if (cleared < 0) > + return BLK_STS_IOERR; > + > + pmem_clear_bb(pmem, to_sect(pmem, offset), cleared >> SECTOR_SHIFT); > + return (cleared < len) ? BLK_STS_IOERR : BLK_STS_OK; I prefer "if / else" syntax instead of a ternary conditional. > } > > static void write_pmem(void *pmem_addr, struct page *page, > @@ -143,7 +169,7 @@ static blk_status_t pmem_do_read(struct pmem_device *pmem, > sector_t sector, unsigned int len) > { > blk_status_t rc; > - phys_addr_t pmem_off = sector * 512 + pmem->data_offset; > + phys_addr_t pmem_off = to_offset(pmem, sector); > void *pmem_addr = pmem->virt_addr + pmem_off; > > if (unlikely(is_bad_pmem(&pmem->bb, sector, len))) > @@ -158,7 +184,7 @@ static blk_status_t pmem_do_write(struct pmem_device *pmem, > struct page *page, unsigned int page_off, > sector_t sector, unsigned int len) > { > - phys_addr_t pmem_off = sector * 512 + pmem->data_offset; > + phys_addr_t pmem_off = to_offset(pmem, sector); > void *pmem_addr = pmem->virt_addr + pmem_off; > > if (unlikely(is_bad_pmem(&pmem->bb, sector, len))) { With those small fixups you can add: Reviewed-by: Dan Williams <dan.j.williams@intel.com>
On 4/11/2022 9:26 PM, Dan Williams wrote: > On Tue, Apr 5, 2022 at 12:48 PM Jane Chu <jane.chu@oracle.com> wrote: >> >> Refactor the pmem_clear_poison() in order to share common code >> later. >> > > I would just add a note here about why, i.e. to factor out the common > shared code between the typical write path and the recovery write > path. Okay. > >> Signed-off-by: Jane Chu <jane.chu@oracle.com> >> --- >> drivers/nvdimm/pmem.c | 78 ++++++++++++++++++++++++++++--------------- >> 1 file changed, 52 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c >> index 0400c5a7ba39..56596be70400 100644 >> --- a/drivers/nvdimm/pmem.c >> +++ b/drivers/nvdimm/pmem.c >> @@ -45,10 +45,27 @@ static struct nd_region *to_region(struct pmem_device *pmem) >> return to_nd_region(to_dev(pmem)->parent); >> } >> >> -static void hwpoison_clear(struct pmem_device *pmem, >> - phys_addr_t phys, unsigned int len) >> +static phys_addr_t to_phys(struct pmem_device *pmem, phys_addr_t offset) >> { >> + return (pmem->phys_addr + offset); > > Christoph already mentioned dropping the unnecessary parenthesis. > >> +} >> + >> +static sector_t to_sect(struct pmem_device *pmem, phys_addr_t offset) >> +{ >> + return (offset - pmem->data_offset) >> SECTOR_SHIFT; >> +} >> + >> +static phys_addr_t to_offset(struct pmem_device *pmem, sector_t sector) >> +{ >> + return ((sector << SECTOR_SHIFT) + pmem->data_offset); >> +} >> + >> +static void pmem_clear_hwpoison(struct pmem_device *pmem, phys_addr_t offset, >> + unsigned int len) > > Perhaps now is a good time to rename this to something else like > pmem_clear_mce_nospec()? Just to make it more distinct from > pmem_clear_poison(). While "hwpoison" is the page flag name > pmem_clear_poison() is the function that's actually clearing the > poison in hardware ("hw") and the new pmem_clear_mce_nospec() is > toggling the page back into service. I get your point. How about calling the function explicitly pmem_mkpage_present()? or pmem_page_present_on()? or pmem_set_page_present()? because setting a poisoned present requires both clearing the HWpoison bit & clear mce_nospec. > >> +{ >> + phys_addr_t phys = to_phys(pmem, offset); >> unsigned long pfn_start, pfn_end, pfn; >> + unsigned int blks = len >> SECTOR_SHIFT; >> >> /* only pmem in the linear map supports HWPoison */ >> if (is_vmalloc_addr(pmem->virt_addr)) >> @@ -67,35 +84,44 @@ static void hwpoison_clear(struct pmem_device *pmem, >> if (test_and_clear_pmem_poison(page)) >> clear_mce_nospec(pfn); >> } >> + >> + dev_dbg(to_dev(pmem), "%#llx clear %u sector%s\n", >> + (unsigned long long) to_sect(pmem, offset), blks, >> + blks > 1 ? "s" : ""); > > In anticipation of better tracing support and the fact that this is no > longer called from pmem_clear_poison() let's drop it for now. OKay. > >> } >> >> -static blk_status_t pmem_clear_poison(struct pmem_device *pmem, >> +static void pmem_clear_bb(struct pmem_device *pmem, sector_t sector, long blks) >> +{ >> + if (blks == 0) >> + return; >> + badblocks_clear(&pmem->bb, sector, blks); >> + if (pmem->bb_state) >> + sysfs_notify_dirent(pmem->bb_state); >> +} >> + >> +static long __pmem_clear_poison(struct pmem_device *pmem, >> phys_addr_t offset, unsigned int len) >> { >> - struct device *dev = to_dev(pmem); >> - sector_t sector; >> - long cleared; >> - blk_status_t rc = BLK_STS_OK; >> - >> - sector = (offset - pmem->data_offset) / 512; >> - >> - cleared = nvdimm_clear_poison(dev, pmem->phys_addr + offset, len); >> - if (cleared < len) >> - rc = BLK_STS_IOERR; >> - if (cleared > 0 && cleared / 512) { >> - hwpoison_clear(pmem, pmem->phys_addr + offset, cleared); >> - cleared /= 512; >> - dev_dbg(dev, "%#llx clear %ld sector%s\n", >> - (unsigned long long) sector, cleared, >> - cleared > 1 ? "s" : ""); >> - badblocks_clear(&pmem->bb, sector, cleared); >> - if (pmem->bb_state) >> - sysfs_notify_dirent(pmem->bb_state); >> + phys_addr_t phys = to_phys(pmem, offset); >> + long cleared = nvdimm_clear_poison(to_dev(pmem), phys, len); >> + >> + if (cleared > 0) { >> + pmem_clear_hwpoison(pmem, offset, cleared); >> + arch_invalidate_pmem(pmem->virt_addr + offset, len); >> } >> + return cleared; >> +} >> >> - arch_invalidate_pmem(pmem->virt_addr + offset, len); >> +static blk_status_t pmem_clear_poison(struct pmem_device *pmem, >> + phys_addr_t offset, unsigned int len) >> +{ >> + long cleared = __pmem_clear_poison(pmem, offset, len); >> >> - return rc; >> + if (cleared < 0) >> + return BLK_STS_IOERR; >> + >> + pmem_clear_bb(pmem, to_sect(pmem, offset), cleared >> SECTOR_SHIFT); >> + return (cleared < len) ? BLK_STS_IOERR : BLK_STS_OK; > > I prefer "if / else" syntax instead of a ternary conditional. > Got it. >> } >> >> static void write_pmem(void *pmem_addr, struct page *page, >> @@ -143,7 +169,7 @@ static blk_status_t pmem_do_read(struct pmem_device *pmem, >> sector_t sector, unsigned int len) >> { >> blk_status_t rc; >> - phys_addr_t pmem_off = sector * 512 + pmem->data_offset; >> + phys_addr_t pmem_off = to_offset(pmem, sector); >> void *pmem_addr = pmem->virt_addr + pmem_off; >> >> if (unlikely(is_bad_pmem(&pmem->bb, sector, len))) >> @@ -158,7 +184,7 @@ static blk_status_t pmem_do_write(struct pmem_device *pmem, >> struct page *page, unsigned int page_off, >> sector_t sector, unsigned int len) >> { >> - phys_addr_t pmem_off = sector * 512 + pmem->data_offset; >> + phys_addr_t pmem_off = to_offset(pmem, sector); >> void *pmem_addr = pmem->virt_addr + pmem_off; >> >> if (unlikely(is_bad_pmem(&pmem->bb, sector, len))) { > > With those small fixups you can add: > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> Thanks! -jane
On Wed, Apr 13, 2022 at 5:55 PM Jane Chu <jane.chu@oracle.com> wrote: > > On 4/11/2022 9:26 PM, Dan Williams wrote: > > On Tue, Apr 5, 2022 at 12:48 PM Jane Chu <jane.chu@oracle.com> wrote: > >> > >> Refactor the pmem_clear_poison() in order to share common code > >> later. > >> > > > > I would just add a note here about why, i.e. to factor out the common > > shared code between the typical write path and the recovery write > > path. > > Okay. > > > > >> Signed-off-by: Jane Chu <jane.chu@oracle.com> > >> --- > >> drivers/nvdimm/pmem.c | 78 ++++++++++++++++++++++++++++--------------- > >> 1 file changed, 52 insertions(+), 26 deletions(-) > >> > >> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c > >> index 0400c5a7ba39..56596be70400 100644 > >> --- a/drivers/nvdimm/pmem.c > >> +++ b/drivers/nvdimm/pmem.c > >> @@ -45,10 +45,27 @@ static struct nd_region *to_region(struct pmem_device *pmem) > >> return to_nd_region(to_dev(pmem)->parent); > >> } > >> > >> -static void hwpoison_clear(struct pmem_device *pmem, > >> - phys_addr_t phys, unsigned int len) > >> +static phys_addr_t to_phys(struct pmem_device *pmem, phys_addr_t offset) > >> { > >> + return (pmem->phys_addr + offset); > > > > Christoph already mentioned dropping the unnecessary parenthesis. > > > >> +} > >> + > >> +static sector_t to_sect(struct pmem_device *pmem, phys_addr_t offset) > >> +{ > >> + return (offset - pmem->data_offset) >> SECTOR_SHIFT; > >> +} > >> + > >> +static phys_addr_t to_offset(struct pmem_device *pmem, sector_t sector) > >> +{ > >> + return ((sector << SECTOR_SHIFT) + pmem->data_offset); > >> +} > >> + > >> +static void pmem_clear_hwpoison(struct pmem_device *pmem, phys_addr_t offset, > >> + unsigned int len) > > > > Perhaps now is a good time to rename this to something else like > > pmem_clear_mce_nospec()? Just to make it more distinct from > > pmem_clear_poison(). While "hwpoison" is the page flag name > > pmem_clear_poison() is the function that's actually clearing the > > poison in hardware ("hw") and the new pmem_clear_mce_nospec() is > > toggling the page back into service. > > I get your point. How about calling the function explicitly > pmem_mkpage_present()? Sure, I like pmem_mkpage_present().
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c index 0400c5a7ba39..56596be70400 100644 --- a/drivers/nvdimm/pmem.c +++ b/drivers/nvdimm/pmem.c @@ -45,10 +45,27 @@ static struct nd_region *to_region(struct pmem_device *pmem) return to_nd_region(to_dev(pmem)->parent); } -static void hwpoison_clear(struct pmem_device *pmem, - phys_addr_t phys, unsigned int len) +static phys_addr_t to_phys(struct pmem_device *pmem, phys_addr_t offset) { + return (pmem->phys_addr + offset); +} + +static sector_t to_sect(struct pmem_device *pmem, phys_addr_t offset) +{ + return (offset - pmem->data_offset) >> SECTOR_SHIFT; +} + +static phys_addr_t to_offset(struct pmem_device *pmem, sector_t sector) +{ + return ((sector << SECTOR_SHIFT) + pmem->data_offset); +} + +static void pmem_clear_hwpoison(struct pmem_device *pmem, phys_addr_t offset, + unsigned int len) +{ + phys_addr_t phys = to_phys(pmem, offset); unsigned long pfn_start, pfn_end, pfn; + unsigned int blks = len >> SECTOR_SHIFT; /* only pmem in the linear map supports HWPoison */ if (is_vmalloc_addr(pmem->virt_addr)) @@ -67,35 +84,44 @@ static void hwpoison_clear(struct pmem_device *pmem, if (test_and_clear_pmem_poison(page)) clear_mce_nospec(pfn); } + + dev_dbg(to_dev(pmem), "%#llx clear %u sector%s\n", + (unsigned long long) to_sect(pmem, offset), blks, + blks > 1 ? "s" : ""); } -static blk_status_t pmem_clear_poison(struct pmem_device *pmem, +static void pmem_clear_bb(struct pmem_device *pmem, sector_t sector, long blks) +{ + if (blks == 0) + return; + badblocks_clear(&pmem->bb, sector, blks); + if (pmem->bb_state) + sysfs_notify_dirent(pmem->bb_state); +} + +static long __pmem_clear_poison(struct pmem_device *pmem, phys_addr_t offset, unsigned int len) { - struct device *dev = to_dev(pmem); - sector_t sector; - long cleared; - blk_status_t rc = BLK_STS_OK; - - sector = (offset - pmem->data_offset) / 512; - - cleared = nvdimm_clear_poison(dev, pmem->phys_addr + offset, len); - if (cleared < len) - rc = BLK_STS_IOERR; - if (cleared > 0 && cleared / 512) { - hwpoison_clear(pmem, pmem->phys_addr + offset, cleared); - cleared /= 512; - dev_dbg(dev, "%#llx clear %ld sector%s\n", - (unsigned long long) sector, cleared, - cleared > 1 ? "s" : ""); - badblocks_clear(&pmem->bb, sector, cleared); - if (pmem->bb_state) - sysfs_notify_dirent(pmem->bb_state); + phys_addr_t phys = to_phys(pmem, offset); + long cleared = nvdimm_clear_poison(to_dev(pmem), phys, len); + + if (cleared > 0) { + pmem_clear_hwpoison(pmem, offset, cleared); + arch_invalidate_pmem(pmem->virt_addr + offset, len); } + return cleared; +} - arch_invalidate_pmem(pmem->virt_addr + offset, len); +static blk_status_t pmem_clear_poison(struct pmem_device *pmem, + phys_addr_t offset, unsigned int len) +{ + long cleared = __pmem_clear_poison(pmem, offset, len); - return rc; + if (cleared < 0) + return BLK_STS_IOERR; + + pmem_clear_bb(pmem, to_sect(pmem, offset), cleared >> SECTOR_SHIFT); + return (cleared < len) ? BLK_STS_IOERR : BLK_STS_OK; } static void write_pmem(void *pmem_addr, struct page *page, @@ -143,7 +169,7 @@ static blk_status_t pmem_do_read(struct pmem_device *pmem, sector_t sector, unsigned int len) { blk_status_t rc; - phys_addr_t pmem_off = sector * 512 + pmem->data_offset; + phys_addr_t pmem_off = to_offset(pmem, sector); void *pmem_addr = pmem->virt_addr + pmem_off; if (unlikely(is_bad_pmem(&pmem->bb, sector, len))) @@ -158,7 +184,7 @@ static blk_status_t pmem_do_write(struct pmem_device *pmem, struct page *page, unsigned int page_off, sector_t sector, unsigned int len) { - phys_addr_t pmem_off = sector * 512 + pmem->data_offset; + phys_addr_t pmem_off = to_offset(pmem, sector); void *pmem_addr = pmem->virt_addr + pmem_off; if (unlikely(is_bad_pmem(&pmem->bb, sector, len))) {
Refactor the pmem_clear_poison() in order to share common code later. Signed-off-by: Jane Chu <jane.chu@oracle.com> --- drivers/nvdimm/pmem.c | 78 ++++++++++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 26 deletions(-)