Re: [PATCH v7 5/6] pmem: refactor pmem_clear_poison()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 4/11/2022 9:26 PM, Dan Williams wrote:
> On Tue, Apr 5, 2022 at 12:48 PM Jane Chu <jane.chu@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>   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@xxxxxxxxx>

Thanks!
-jane





[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux