Re: [PATCH v2 0/3] memory: memory_region_is_mapped() cleanups

[Peter Xu]

On Wed, Oct 27, 2021 at 09:12:08AM +0200, David Hildenbrand wrote:
> On 27.10.21 05:53, Peter Xu wrote:
> > On Tue, Oct 26, 2021 at 06:06:46PM +0200, David Hildenbrand wrote:
> >> This is the follow-up of [1].
> >>
> >> Playing with memory_region_is_mapped(), I realized that memory regions
> >> mapped via an alias behave a little bit "differently", as they don't have
> >> their ->container set.
> > 
> 
> Hi Peter,
> 
> thanks for your review!
> 
> > The patches look ok to me, though I have a few pure questions to ask..
> > 
> >> * memory_region_is_mapped() will never succeed for memory regions mapped
> >>   via an alias
> > 
> > I think you mentioned that in commit message of patch 2 that it fixes no 
> > real
> > problem so far, so I'm also wondering in which case it'll help.  Say, 
> > normally
> > when there's an alias of another MR and we want to know whether the MR is
> > mapped, we simply call memory_region_is_mapped() upon the alias .
> 
> Just to recap: in v1 I proposed to just document that it doesn't work on
> aliases, and folks weren't too happy to see regions mapped via aliases
> being special-cased where it might just be avoided.
> 
> > 
> > To verify my thoughts, I did look up a few memory_region_is_mapped() random
> > callers that used with alias and that's what they did:
> > 
> > Here'sthe dino.c example:
> > 
> > *** hw/hppa/dino.c:
> > gsc_to_pci_forwarding[151]     if (!memory_region_is_mapped(mem)) {
> > gsc_to_pci_forwarding[155]     } else if (memory_region_is_mapped(mem)) {
> > 
> > The "mem" points to:
> > 
> >         MemoryRegion *mem = &s->pci_mem_alias[i];
> > 
> > Which is the alias.
> > 
> > Another one:
> > 
> > *** hw/pci-host/pnv_phb3.c:
> > pnv_phb3_check_m32[121]        if (memory_region_is_mapped(&phb->mr_m32)) {
> > pnv_phb3_update_regions[1076]  if (memory_region_is_mapped(&phb->mr_m32)) {
> > 
> > Andmr_m32 is the alias MR itself:
> > 
> >     memory_region_init_alias(&phb->mr_m32, OBJECT(phb), "phb3-m32",
> >                              &phb->pci_mmio, start, size);
> > 
> > I mean, if it should always be very straightforward to fetch the alias mr, 
> > then
> > I'm just afraid patch 2 won't really help in any real use case but pure 
> > overhead.
> 
> That is true as long as it's not a mapping check in generic code. Say,
> we have a RAMBlock and use ->mr. Checking
> memory_region_is_mapped(rb->mr) is misleading if the region is mapped
> via aliases.
> 
> The reason I stumbled over this at all is a sanity check I added in
> 
> void memory_region_set_ram_discard_manager(MemoryRegion *mr,
>                                            RamDiscardManager *rdm)
> {
>     g_assert(memory_region_is_ram(mr) && !memory_region_is_mapped(mr));
>     g_assert(!rdm || !mr->rdm);
>     mr->rdm = rdm;
> }
> 
> If mr is only mapped via aliases (see the virtio-mem memslot series),
> this check is of no value, because even if the mr would be mapped via
> aliases, we would not be able to catch it.

Yeah, this is a sane check to ask for.

> 
> Having that said, the check is not 100% correct, because
> memory_region_is_mapped() does not indicate that we're actually mapped
> into an address space. But at least for memory devices (-> target use
> case of RamDiscardManager) with an underlying RAMBlock, it's pretty
> reliable -- and there is no easy way to check if we're mapped into an
> address space when aliases are involved.
> 
> Note that there is also a similar check in memory_region_is_mapped(),
> but I'm removing that as part of the virtio-mem memslot series, because
> it's not actually helpful in the context of RAMBlock migration (nothing
> might be mapped, but migration code would still try migrating such a
> RAMBlock and has to consider the RamDiscardManager).
> 
> 
> Another example of a generic code check is patch #1: the only reason it
> works right now is because NVDIMMs cannot exist before initial memory is
> created. But yes, there is a better way of doing it when we have a
> memdev at hand.

IMHO patch 1 is actually an example showing that when we want that explicit
meaning we can simply introduce another boolean in parent struct. :)

> 
> > 
> > And I hope we won't trigger problem with any use case where
> > memory_region_is_mapped() returned false previously but then it'll return 
> > true
> > after patch 2, because logically with the old code one can detect 
> > explicitly on
> > "whether this original MR is mapped somewhere, irrelevant of other alias
> > mappings upon this mr".  Patch 2 blurrs it from that pov.
> > 
> 
> I hope tests will catch that early. I ran some without surprises.
> 
> >> * memory_region_to_address_space(), memory_region_find(),
> >>   memory_region_find_rcu(), memory_region_present() won't work, which seems
> >>   okay, because we don't expect such memory regions getting passed to these
> >>   functions.
> > 
> > Looks right.
> > 
> >> * memory_region_to_absolute_addr() will result in a wrong address. As
> >>   the result is only used for tracing, that is tolerable.
> > 
> > memory_region_{read|write}_accessor() seem to be only called from the 
> > address
> > space layer, so it looks fine even for tracing as it'll always fetch the 
> > alias,
> > afaiu.  Phil's patch may change that fact, though, it seems.
> 
> Unfortunately, not much we can do: a memory region might theoretically
> be mapped via aliases into different address spaces and into different
> locations: there is no right answer to memory_region_to_absolute_addr()
> when only having the memory region at hand without an address space.

Yes.

Now I think patch 2 is fine too, it'll be nicer imho to be a new API like
memory_region_is_mapped_any() with comment showing it checks the aliases, but
no strong opinion.  If nothing fails with the change, it'll be the same indeed.

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu