On Tue, Jul 16, 2019 at 3:29 PM Nadav Amit <namit@xxxxxxxxxx> wrote: > > > On Jul 16, 2019, at 3:20 PM, Dan Williams <dan.j.williams@xxxxxxxxx> wrote: > > > > On Tue, Jul 16, 2019 at 3:13 PM Nadav Amit <namit@xxxxxxxxxx> wrote: > >>> On Jul 16, 2019, at 3:07 PM, Dan Williams <dan.j.williams@xxxxxxxxx> wrote: > >>> > >>> On Tue, Jul 16, 2019 at 3:01 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: > >>>> On Tue, 18 Jun 2019 21:56:43 +0000 Nadav Amit <namit@xxxxxxxxxx> wrote: > >>>> > >>>>>> ...and is constant for the life of the device and all subsequent mappings. > >>>>>> > >>>>>>> Perhaps you want to cache the cachability-mode in vma->vm_page_prot (which I > >>>>>>> see being done in quite a few cases), but I don’t know the code well enough > >>>>>>> to be certain that every vma should have a single protection and that it > >>>>>>> should not change afterwards. > >>>>>> > >>>>>> No, I'm thinking this would naturally fit as a property hanging off a > >>>>>> 'struct dax_device', and then create a version of vmf_insert_mixed() > >>>>>> and vmf_insert_pfn_pmd() that bypass track_pfn_insert() to insert that > >>>>>> saved value. > >>>>> > >>>>> Thanks for the detailed explanation. I’ll give it a try (the moment I find > >>>>> some free time). I still think that patch 2/3 is beneficial, but based on > >>>>> your feedback, patch 3/3 should be dropped. > >>>> > >>>> It has been a while. What should we do with > >>>> > >>>> resource-fix-locking-in-find_next_iomem_res.patch > >>> > >>> This one looks obviously correct to me, you can add: > >>> > >>> Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx> > >>> > >>>> resource-avoid-unnecessary-lookups-in-find_next_iomem_res.patch > >>> > >>> This one is a good bug report that we need to go fix pgprot lookups > >>> for dax, but I don't think we need to increase the trickiness of the > >>> core resource lookup code in the meantime. > >> > >> I think that traversing big parts of the tree that are known to be > >> irrelevant is wasteful no matter what, and this code is used in other cases. > >> > >> I don’t think the new code is so tricky - can you point to the part of the > >> code that you find tricky? > > > > Given dax can be updated to avoid this abuse of find_next_iomem_res(), > > it was a general observation that the patch adds more lines than it > > removes and is not strictly necessary. I'm ambivalent as to whether it > > is worth pushing upstream. If anything the changelog is going to be > > invalidated by a change to dax to avoid find_next_iomem_res(). Can you > > update the changelog to be relevant outside of the dax case? > > Well, 8 lines are comments, 4 are empty lines, so it adds 3 lines of code > according to my calculations. :) > > Having said that, if you think I might have made a mistake, or you are > concerned with some bug I might have caused, please let me know. I > understand that this logic might have been lying around for some time. Like I said, I'm ambivalent and not NAK'ing it. It looks ok, but at the same time something is wrong if a hot path is constantly re-looking up a resource. The fact that it shows up in profiles when that happens could be considered useful. > I can update the commit log, emphasizing the redundant search operations as > motivation and then mentioning dax as an instance that induces overheads due > to this overhead, and say it should be handled regardless to this patch-set. > Once I find time, I am going to deal with DAX, unless you beat me to it. It turns out that the ability to ask the driver for pgprot bits is useful above and beyond this performance optimization. For example I'm looking to use it to support disabling speculation into pages with known media errors by letting the driver consult its 'badblock' list. There are also usages for passing the key-id for persistent memory encrypted by MKTME.
