xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>
Subject: Re: [PATCH v3 01/22] mm: introduce xvmalloc() et al and use for grant table allocations
Date: Mon, 3 May 2021 18:39:12 +0200	[thread overview]
Message-ID: <YJAnMAmob1Y5myp4@Air-de-Roger> (raw)
In-Reply-To: <340fed73-973c-feba-074d-8bfa6eeae6d6@suse.com>

On Mon, May 03, 2021 at 05:21:37PM +0200, Jan Beulich wrote:
> On 03.05.2021 16:54, Roger Pau Monné wrote:
> > On Mon, May 03, 2021 at 03:50:48PM +0200, Jan Beulich wrote:
> >> On 03.05.2021 13:31, Roger Pau Monné wrote:
> >>> On Thu, Apr 22, 2021 at 04:43:39PM +0200, Jan Beulich wrote:
> >>>> All of the array allocations in grant_table_init() can exceed a page's
> >>>> worth of memory, which xmalloc()-based interfaces aren't really suitable
> >>>> for after boot. We also don't need any of these allocations to be
> >>>> physically contiguous.. Introduce interfaces dynamically switching
> >>>> between xmalloc() et al and vmalloc() et al, based on requested size,
> >>>> and use them instead.
> >>>>
> >>>> All the wrappers in the new header get cloned mostly verbatim from
> >>>> xmalloc.h, with the sole adjustment to switch unsigned long to size_t
> >>>> for sizes and to unsigned int for alignments.
> >>>
> >>> We seem to be growing a non-trivial amount of memory allocation
> >>> families of functions: xmalloc, vmalloc and now xvmalloc.
> >>>
> >>> I think from a consumer PoV it would make sense to only have two of
> >>> those: one for allocations that require to be physically contiguous,
> >>> and one for allocation that don't require it.
> >>>
> >>> Even then, requesting for physically contiguous allocations could be
> >>> done by passing a flag to the same interface that's used for
> >>> non-contiguous allocations.
> >>>
> >>> Maybe another option would be to expand the existing
> >>> v{malloc,realloc,...} set of functions to have your proposed behaviour
> >>> for xv{malloc,realloc,...}?
> >>
> >> All of this and some of your remarks further down has already been
> >> discussed. A working group has been formed. No progress since. Yes,
> >> a smaller set of interfaces may be the way to go. Controlling
> >> behavior via flags, otoh, is very much not malloc()-like. Making
> >> existing functions have the intended new behavior is a no-go without
> >> auditing all present uses, to find those few which actually may need
> >> physically contiguous allocations.
> > 
> > But you could make your proposed xvmalloc logic the implementation
> > behind vmalloc, as that would still be perfectly fine and safe? (ie:
> > existing users of vmalloc already expect non-physically contiguous
> > memory). You would just optimize the size < PAGE_SIZE for that
> > interface?
> 
> Existing callers of vmalloc() may expect page alignment of the
> returned address.

Right - just looked and also the interface is different from
x{v}malloc, so you would have to fixup callers.

> >>>> --- /dev/null
> >>>> +++ b/xen/include/xen/xvmalloc.h
> >>>> @@ -0,0 +1,73 @@
> >>>> +
> >>>> +#ifndef __XVMALLOC_H__
> >>>> +#define __XVMALLOC_H__
> >>>> +
> >>>> +#include <xen/cache.h>
> >>>> +#include <xen/types.h>
> >>>> +
> >>>> +/*
> >>>> + * Xen malloc/free-style interface for allocations possibly exceeding a page's
> >>>> + * worth of memory, as long as there's no need to have physically contiguous
> >>>> + * memory allocated.  These should be used in preference to xmalloc() et al
> >>>> + * whenever the size is not known to be constrained to at most a single page.
> >>>
> >>> Even when it's know that size <= PAGE_SIZE this helpers are
> >>> appropriate as they would end up using xmalloc, so I think it's fine to
> >>> recommend them universally as long as there's no need to alloc
> >>> physically contiguous memory?
> >>>
> >>> Granted there's a bit more overhead from the logic to decide between
> >>> using xmalloc or vmalloc &c, but that's IMO not that big of a deal in
> >>> order to not recommend this interface globally for non-contiguous
> >>> alloc.
> >>
> >> As long as xmalloc() and vmalloc() are meant stay around as separate
> >> interfaces, I wouldn't want to "forbid" their use when it's sufficiently
> >> clear that they would be chosen by the new function anyway. Otoh, if the
> >> new function became more powerful in terms of falling back to the
> > 
> > What do you mean with more powerful here?
> 
> Well, right now the function is very simplistic, looking just at the size
> and doing no fallback attempts at all. Linux'es kvmalloc() goes a little
> farther. What I see as an option is for either form of allocation to fall
> back to the other form in case the first attempt fails. This would cover
> - out of memory Xen heap for small allocs,
> - out of VA space for large allocs.
> And of course, like Linux does (or at least did at the time I looked at
> their code), the choice which of the backing functions to call could also
> become more sophisticated over time.

I'm not opposed to any of this, but even your proposed code right now
seems no worse than using either vmalloc or xmalloc, as it's only a
higher level wrapper around those.

What I would prefer is to propose to use function foo for all
allocations that don't require contiguous physical memory, and
function bar for those that do require contiguous physical memory.
It's IMO awkward from a developer PoV to have to select an
allocation function based on the size to be allocated.

I wouldn't mind if you wanted to name this more generic wrapped straight
malloc.

Thanks, Roger.


  reply	other threads:[~2021-05-03 16:39 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-22 14:38 [PATCH v3 00/22] xvmalloc() / x86 xstate area / x86 CPUID / AMX+XFD Jan Beulich
2021-04-22 14:43 ` [PATCH v3 01/22] mm: introduce xvmalloc() et al and use for grant table allocations Jan Beulich
2021-05-03 11:31   ` Roger Pau Monné
2021-05-03 13:50     ` Jan Beulich
2021-05-03 14:54       ` Roger Pau Monné
2021-05-03 15:21         ` Jan Beulich
2021-05-03 16:39           ` Roger Pau Monné [this message]
2021-04-22 14:44 ` [PATCH v3 02/22] x86/xstate: use xvzalloc() for save area allocation Jan Beulich
2021-05-05 13:29   ` Roger Pau Monné
2021-04-22 14:44 ` [PATCH v3 03/22] x86/xstate: re-size save area when CPUID policy changes Jan Beulich
2021-05-03 13:57   ` Andrew Cooper
2021-05-03 14:22     ` Jan Beulich
2021-05-11 16:41       ` Andrew Cooper
2021-05-17  7:33         ` Jan Beulich
2021-04-22 14:45 ` [PATCH v3 04/22] x86/xstate: re-use valid_xcr0() for boot-time checks Jan Beulich
2021-05-03 11:53   ` Andrew Cooper
2021-04-22 14:45 ` [PATCH v3 05/22] x86/xstate: drop xstate_offsets[] and xstate_sizes[] Jan Beulich
2021-05-03 16:10   ` Andrew Cooper
2021-05-04  7:57     ` Jan Beulich
2021-04-22 14:46 ` [PATCH v3 06/22] x86/xstate: replace xsave_cntxt_size and drop XCNTXT_MASK Jan Beulich
2021-04-22 14:47 ` [PATCH v3 07/22] x86/xstate: avoid accounting for unsupported components Jan Beulich
2021-04-22 14:47 ` [PATCH v3 08/22] x86: use xvmalloc() for extended context buffer allocations Jan Beulich
2021-04-22 14:48 ` [PATCH v3 09/22] x86/xstate: enable AMX components Jan Beulich
2021-04-22 14:50 ` [PATCH v3 10/22] x86/CPUID: adjust extended leaves out of range clearing Jan Beulich
2021-04-22 14:50 ` [PATCH v3 11/22] x86/CPUID: move bounding of max_{,sub}leaf fields to library code Jan Beulich
2021-04-22 14:51 ` [PATCH v3 12/22] x86/CPUID: enable AMX leaves Jan Beulich
2021-04-22 14:52 ` [PATCH v3 13/22] x86: XFD enabling Jan Beulich
2021-04-22 14:53 ` [PATCH v3 14/22] x86emul: introduce X86EMUL_FPU_{tilecfg,tile} Jan Beulich
2021-04-22 14:53 ` [PATCH v3 15/22] x86emul: support TILERELEASE Jan Beulich
2021-04-22 14:53 ` [PATCH v3 16/22] x86: introduce struct for TILECFG register Jan Beulich
2021-04-22 14:54 ` [PATCH v3 17/22] x86emul: support {LD,ST}TILECFG Jan Beulich
2021-04-22 14:55 ` [PATCH v3 18/22] x86emul: support TILEZERO Jan Beulich
2021-04-22 14:55 ` [PATCH v3 19/22] x86emul: support TILELOADD{,T1} and TILESTORE Jan Beulich
2021-04-22 15:06   ` Jan Beulich
2021-04-22 15:11     ` Jan Beulich
2021-04-26  7:12       ` Paul Durrant
2021-04-29  9:40         ` Jan Beulich
2021-04-22 14:56 ` [PATCH v3 20/22] x86emul: support tile multiplication insns Jan Beulich
2021-04-22 14:57 ` [PATCH v3 21/22] x86emul: test AMX insns Jan Beulich
2021-04-22 14:57 ` [PATCH v3 22/22] x86: permit guests to use AMX and XFD Jan Beulich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YJAnMAmob1Y5myp4@Air-de-Roger \
    --to=roger.pau@citrix.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=george.dunlap@citrix.com \
    --cc=iwj@xenproject.org \
    --cc=jbeulich@suse.com \
    --cc=julien@xen.org \
    --cc=sstabellini@kernel.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).