Re: [PATCH ndctl v1 0/8] daxctl: Add device align and range mapping allocation

From: Joao Martins <joao.m.martins@oracle.com>
To: Dan Williams <dan.j.williams@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>
Cc: "linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>
Subject: Re: [PATCH ndctl v1 0/8] daxctl: Add device align and range mapping allocation
Date: Thu, 17 Dec 2020 11:23:10 +0000	[thread overview]
Message-ID: <b5a6577f-0384-8cbf-d3c7-2512f0f5d22c@oracle.com> (raw)
In-Reply-To: <CAPcyv4jQ9ee-o3t_Wy_4K-Nm5Lj6wJKv1pdjvtYQG6eJ3ejUrQ@mail.gmail.com>

On 12/16/20 11:42 PM, Dan Williams wrote:
> On Wed, Dec 16, 2020 at 2:54 PM Joao Martins <joao.m.martins@oracle.com> wrote:
>> On 12/16/20 10:31 PM, Dan Williams wrote:
>>> On Wed, Dec 16, 2020 at 1:51 PM Joao Martins <joao.m.martins@oracle.com> wrote:
>>>> On 12/16/20 6:42 PM, Verma, Vishal L wrote:
>>>>> On Wed, 2020-12-16 at 11:39 +0000, Joao Martins wrote:
>>>>>> On 7/16/20 7:46 PM, Joao Martins wrote:
>>>>>>> Hey,
>>>>>>>
>>>>>>> This series builds on top of this one[0] and does the following improvements
>>>>>>> to the Soft-Reserved subdivision:
>>>>>>>
>>>>>>>  1) Support for {create,reconfigure}-device for selecting @align (hugepage size).
>>>>>>>  Here we add a '-a|--align 4K|2M|1G' option to the existing commands;
>>>>>>>
>>>>>>>  2) Listing improvements for device alignment and mappings;
>>>>>>>  Note: Perhaps it is better to hide the mappings by default, and only
>>>>>>>        print with -v|--verbose. This would align with ndctl, as the mappings
>>>>>>>        info can be quite large.
>>>>>>>
>>>>>>>  3) Allow creating devices from selecting ranges. This allows to keep the
>>>>>>>    same GPA->HPA mapping as before we kexec the hypervisor with running guests:
>>>>>>>
>>>>>>>    daxctl list -d dax0.1 > /var/log/dax0.1.json
>>>>>>>    kexec -d -l bzImage
>>>>>>>    systemctl kexec
>>>>>>>    daxctl create -u --restore /var/log/dax0.1.json
>>>>>>>
>>>>>>>    The JSON was what I though it would be easier for an user, given that it is
>>>>>>>    the data format daxctl outputs. Alternatives could be adding multiple:
>>>>>>>     --mapping <pgoff>:<start>-<end>
>>>>>>>
>>>>>>>    But that could end up in a gigantic line and a little more
>>>>>>>    unmanageable I think.
>>>>>>>
>>>>>>> This series requires this series[0] on top of Dan's patches[1]:
>>>>>>>
>>>>>>>  [0] https://lore.kernel.org/linux-nvdimm/20200716172913.19658-1-joao.m.martins@oracle.com/
>>>>>>>  [1] https://lore.kernel.org/linux-nvdimm/159457116473.754248.7879464730875147365.stgit@dwillia2-desk3.amr.corp.intel.com/
>>>>>>>
>>>>>>> The only TODO here is docs and improving tests to validate mappings, and test
>>>>>>> the restore path.
>>>>>>>
>>>>>>> Suggestions/comments are welcome.
>>>>>>>
>>>>>> There's a couple of issues in this series regarding daxctl-reconfigure options and
>>>>>> breakage of ndctl with kernels (<5.10) that do not supply a device @align upon testing
>>>>>> with NVDIMMs. Plus it is missing daxctl-create.sh unit test for @align.
>>>>>>
>>>>>> I will fix those and respin, and probably take out the last patch as it's more RFC-ish and
>>>>>> in need of feedback.
>>>>>
>>>>> Sounds good. Any objections to releasing v70 with the initial support,
>>>>> and then adding this series on for the next one? I'm thinking I'll do a
>>>>> much quicker v72 release say in early January with this and anything
>>>>> else that missed v71.
>>>>
>>>> If we're able to wait until tomorrow, I could respin these first four patches with the
>>>> fixes and include the align support in the initial set. Otherwise, I am also good if you
>>>> prefer defering it to v72.
>>>
>>> Does this change the JSON output? Might be nice to keep that all in
>>> one update rather than invalidate some testing with the old format
>>> betweem v71 and v72.
>>>
>> Ugh, sent the v2 too early before seeing this.
>>
>> The only change to the output is on daxctl when listing devices for 5.10+.
>> It starts displaying an "align" key/value.
> 
> No worries. Vishal and I chatted and it looks to me like your
> improvements to the provisioning flow are worthwhile to sneak into the
> v71 release if you want to include those changes as well.

The provisioning flow additions has some questions open about the daxctl
user interface. To summarize:

1) Should we always list mappings, or only list them with a -v option? Or
maybe instead of -v to use instead a new -M option which enables the
listing of mappings?

The reason being that it can get quite verbose with a device picks a lot
of mappings, hence I would imagine this info isn't necessary for the casual
daxctl listing.

2) Does the '--restore <file.json>' should instead be called it
instead '--device'? I feel the name '--restore' is too tied to one specific
way of using it when the feature can be used by a tool which wants to manage
its own range mappings. Additionally, I was thinking that if one wants to
manually add/fixup ranges more easily that we would add
a --mapping <pgoff>:<start>-<end> sort of syntax. But I suppose this could
be added later if its really desired.

And with these clarified, I could respin it over. Oh and I'm missing a
mappings test as well.

It's worth mentioning that kexec will need fixing, as dax_hmem regions
created with HMAT or manually assigned with efi_fake_mem= get lost on
kexec because we do not pass the EFI_MEMMAP conventional memory ranges
to the second kernel (only runtime code/boot services). I have a
RFC patch for x86 efi handling, but should get that conversation started
after holidays.

	Joao
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org