Re: [RFC PATCH 1/2] libnvdimm: Add prctl control for disabling synchronous fault support.

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

On 5/29/20 3:22 PM, Jan Kara wrote:
> Hi!
> 
> On Fri 29-05-20 15:07:31, Aneesh Kumar K.V wrote:
>> Thanks Michal. I also missed Jeff in this email thread.
> 
> And I think you'll also need some of the sched maintainers for the prctl
> bits...
> 
>> On 5/29/20 3:03 PM, Michal Suchánek wrote:
>>> Adding Jan
>>>
>>> On Fri, May 29, 2020 at 11:11:39AM +0530, Aneesh Kumar K.V wrote:
>>>> With POWER10, architecture is adding new pmem flush and sync instructions.
>>>> The kernel should prevent the usage of MAP_SYNC if applications are not using
>>>> the new instructions on newer hardware.
>>>>
>>>> This patch adds a prctl option MAP_SYNC_ENABLE that can be used to enable
>>>> the usage of MAP_SYNC. The kernel config option is added to allow the user
>>>> to control whether MAP_SYNC should be enabled by default or not.
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ...
>>>> diff --git a/kernel/fork.c b/kernel/fork.c
>>>> index 8c700f881d92..d5a9a363e81e 100644
>>>> --- a/kernel/fork.c
>>>> +++ b/kernel/fork.c
>>>> @@ -963,6 +963,12 @@ __cacheline_aligned_in_smp DEFINE_SPINLOCK(mmlist_lock);
>>>>    static unsigned long default_dump_filter = MMF_DUMP_FILTER_DEFAULT;
>>>> +#ifdef CONFIG_ARCH_MAP_SYNC_DISABLE
>>>> +unsigned long default_map_sync_mask = MMF_DISABLE_MAP_SYNC_MASK;
>>>> +#else
>>>> +unsigned long default_map_sync_mask = 0;
>>>> +#endif
>>>> +
> 
> I'm not sure CONFIG is really the right approach here. For a distro that would
> basically mean to disable MAP_SYNC for all PPC kernels unless application
> explicitly uses the right prctl. Shouldn't we rather initialize
> default_map_sync_mask on boot based on whether the CPU we run on requires
> new flush instructions or not? Otherwise the patch looks sensible.
> 

yes that is correct. We ideally want to deny MAP_SYNC only w.r.t 
POWER10. But on a virtualized platform there is no easy way to detect 
that. We could ideally hook this into the nvdimm driver where we look at 
the new compat string ibm,persistent-memory-v2 and then disable MAP_SYNC
if we find a device with the specific value.

BTW with the recent changes I posted for the nvdimm driver, older kernel 
won't initialize persistent memory device on newer hardware. Newer 
hardware will present the device to OS with a different device tree 
compat string.

My expectation  w.r.t this patch was, Distro would want to  mark
CONFIG_ARCH_MAP_SYNC_DISABLE=n based on the different application 
certification.  Otherwise application will have to end up calling the 
prctl(MMF_DISABLE_MAP_SYNC, 0) any way. If that is the case, should this
be dependent on P10?

With that I am wondering should we even have this patch? Can we expect 
userspace get updated to use new instruction?.

With ppc64 we never had a real persistent memory device available for 
end user to try. The available persistent memory stack was using vPMEM 
which was presented as a volatile memory region for which there is no 
need to use any of the flush instructions. We could safely assume that 
as we get applications certified/verified for working with pmem device 
on ppc64, they would all be using the new instructions?


-aneesh


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