[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 3/3] x86/mem_sharing: make fork_reset more configurable
On Thu, Mar 24, 2022 at 12:44 PM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote: > > On Thu, Mar 24, 2022 at 12:22:49PM -0400, Tamas K Lengyel wrote: > > On Thu, Mar 24, 2022 at 12:04 PM Roger Pau Monné <roger.pau@xxxxxxxxxx> > > wrote: > > > > > > On Thu, Mar 24, 2022 at 11:52:38AM -0400, Tamas K Lengyel wrote: > > > > On Thu, Mar 24, 2022 at 11:46 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> > > > > wrote: > > > > > > > > > > On Tue, Mar 22, 2022 at 01:41:39PM -0400, Tamas K Lengyel wrote: > > > > > > diff --git a/xen/include/public/memory.h > > > > > > b/xen/include/public/memory.h > > > > > > index 208d8dcbd9..30ce23c5a7 100644 > > > > > > --- a/xen/include/public/memory.h > > > > > > +++ b/xen/include/public/memory.h > > > > > > @@ -541,12 +541,14 @@ struct xen_mem_sharing_op { > > > > > > uint32_t gref; /* IN: gref to debug */ > > > > > > } u; > > > > > > } debug; > > > > > > - struct mem_sharing_op_fork { /* OP_FORK */ > > > > > > + struct mem_sharing_op_fork { /* OP_FORK/_RESET */ > > > > > > domid_t parent_domain; /* IN: parent's domain > > > > > > id */ > > > > > > /* These flags only makes sense for short-lived forks */ > > > > > > #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0) > > > > > > #define XENMEM_FORK_BLOCK_INTERRUPTS (1u << 1) > > > > > > #define XENMEM_FORK_SKIP_SPECIAL_PAGES (1u << 2) > > > > > > +#define XENMEM_FORK_RESET_STATE (1u << 3) > > > > > > +#define XENMEM_FORK_RESET_MEMORY (1u << 4) > > > > > > uint16_t flags; /* IN: optional settings > > > > > > */ > > > > > > uint32_t pad; /* Must be set to 0 */ > > > > > > } fork; > > > > > > diff --git a/xen/include/public/vm_event.h > > > > > > b/xen/include/public/vm_event.h > > > > > > index bb003d21d0..81c2ee28cc 100644 > > > > > > --- a/xen/include/public/vm_event.h > > > > > > +++ b/xen/include/public/vm_event.h > > > > > > @@ -127,6 +127,14 @@ > > > > > > * Reset the vmtrace buffer (if vmtrace is enabled) > > > > > > */ > > > > > > #define VM_EVENT_FLAG_RESET_VMTRACE (1 << 13) > > > > > > +/* > > > > > > + * Reset the VM state (if VM is fork) > > > > > > + */ > > > > > > +#define VM_EVENT_FLAG_RESET_FORK_STATE (1 << 14) > > > > > > +/* > > > > > > + * Remove unshared entried from physmap (if VM is fork) > > > > > > + */ > > > > > > +#define VM_EVENT_FLAG_RESET_FORK_MEMORY (1 << 15) > > > > > > > > > > I'm confused about why two different interfaces are added to do this > > > > > kind of selective resets, one to vm_event and one to xenmem_fork? > > > > > > > > > > I thin k the natural place for the option to live would be > > > > > XENMEM_FORK? > > > > > > > > Yes, that's the natural place for it. But we are adding it to both for > > > > a reason. In our use-case the reset operation will happen after a > > > > vm_event is received to which we already must send a reply. Setting > > > > the flag on the vm_event reply saves us having to issue an extra memop > > > > hypercall afterwards. > > > > > > Can you do a multicall and batch both operations in a single > > > hypercall? > > > > > > That would seem more natural than adding duplicated interfaces. > > > > Not in a straight forward way, no. There is no exposed API in libxc to > > do a multicall. Even if that was an option it is still easier for me > > to just flip a bit in the response field than having to construct a > > whole standalone hypercall structure to be sent as part of a > > multicall. > > Right, I can see it being easier, but it seems like a bad choice from > an interface PoV. You are the maintainer of both subsystems, but it > would seem to me it's in your best interest to try to keep the > interfaces separated and clean. > > Would it be possible for the reset XENMEM_FORK op to have the side > effect of performing what you would instead do with the vm_event > hypercall? Yes, the event response is really just an event channel signal to Xen, so the memop hypercall could similarly encode the "now check the vm_event response" as an optional field. But why is that any better than the current event channel route processing the vm_response encoding the "now do these ops on the fork"? We already have a bunch of different operations you can encode in the vm_event response field, so it reduces the complexity on the toolstack side since I don't have to switch around which hypercall I need to issue depending on what extra ops I want to put into a single hypercall. Tamas
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |