Re: [PATCH v8 03/15] x86/mm: rewrite virt_to_xen_l*e

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 18/08/2020 09:49, Jan Beulich wrote:
> On 13.08.2020 19:22, Julien Grall wrote:
> > Hi,
> >
> > On 13/08/2020 17:08, Hongyan Xia wrote:
> > > On Fri, 2020-08-07 at 16:05 +0200, Jan Beulich wrote:
> > > > On 27.07.2020 16:21, Hongyan Xia wrote:
> > > > > From: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > > >
> > > > > Rewrite those functions to use the new APIs. Modify its callers to
> > > > > unmap
> > > > > the pointer returned. Since alloc_xen_pagetable_new() is almost
> > > > > never
> > > > > useful unless accompanied by page clearing and a mapping, introduce
> > > > > a
> > > > > helper alloc_map_clear_xen_pt() for this sequence.
> > > > >
> > > > > Note that the change of virt_to_xen_l1e() also requires
> > > > > vmap_to_mfn() to
> > > > > unmap the page, which requires domain_page.h header in vmap.
> > > > >
> > > > > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > > > Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
> > > > > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > >
> > > > > ---
> > > > > Changed in v8:
> > > > > - s/virtual address/linear address/.
> > > > > - BUG_ON() on NULL return in vmap_to_mfn().
> > > > The justification for this should be recorded in the description. In
> > > Will do.
> > >
> > > > reply to v7 I did even suggest how to easily address the issue you
> > > > did notice with large pages, as well as alternative behavior for
> > > > vmap_to_mfn().
> > > One thing about adding SMALL_PAGES is that vmap is common code and I am
> > > not sure if the Arm side is happy with it.
> > At the moment, Arm is only using small mapping but I plan to change that soon 
> > because we have regions that can be fairly big.
> >
> > Regardless that, the issue with vmap_to_mfn() is rather x86 specific. So I 
> > don't particularly like the idea to expose such trick in common code.
> >
> > Even on x86, I think this is not the right approach. Such band-aid will impact 
> > the performance as, assuming superpages are used, it will take longer to map 
> > and add pressure on the TLBs.
> >
> > I am aware that superpages will be useful for LiveUpdate, but is there any use 
> > cases in upstream?
> Superpage use by vmalloc() is purely occasional: You'd have to vmalloc()
> 2Mb or more _and_ the page-wise allocation ought to return 512
> consecutive pages in the right order. Getting 512 consecutive pages is
> possible in practice, but with the page allocator allocating top-down it
> is very unlikely for them to be returned in increasing-sorted order.
So your assumption here is vmap_to_mfn() can only be called on 
vmalloc-ed() area. While this may be the case in Xen today, the name 
clearly suggest it can be called on all vmap-ed region.
> > If not, could we just use the BUG_ON() and implement correctly vmap_to_mfn() in 
> > a follow-up?
> My main concern with the BUG_ON() is that it detects a problem long after
> it was introduced (when the mapping was established). I'd rather see a
> BUG_ON() added there if use of MAP_SMALL_PAGES is deemed unsuitable.
From what you wrote, I would agree that vmalloc() is unlikely going to 
use superpages. But this is not going to solve the underlying problem 
with the rest of the vmap area.
So are you suggesting to use MAP_SMALL_PAGES for *all* the vmap()?

Cheers,

--
Julien Grall