Re: [Xen-devel] [RFC PATCH 3/6] vm_event: Refactor vm_event_domain implementation

[Petre Ovidiu PIRCALABU <ppircalabu@xxxxxxxxxxxxxxx>]

On Wed, 2018-12-19 at 15:26 -0700, Tamas K Lengyel wrote:
> On Wed, Dec 19, 2018 at 11:52 AM Petre Pircalabu
> <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> > 
> > Decouple the VM Event interface from the ring implementation.
> 
> This will need a much better description. There is also a lot of
> churn
> that is mostly just mechanical in this patch but makes reviewing it
> hard. Perhaps functional changes and mechanical changes could be
> split
> to two patches?
This was a auxiliary patch to help introduce the new
vm_event_domain_channel by making the vm_event interface implementation
agnostic. I will try splitting it in order to make it more readable.

> 
> > +struct vm_event_domain
> > +{
> > +    /* VM event ops */
> > +    bool (*check)(struct vm_event_domain *ved);
> > +    int (*claim_slot)(struct vm_event_domain *ved, bool
> > allow_sleep);
> > +    void (*release_slot)(struct vm_event_domain *ved);
> > +    void (*put_request)(struct vm_event_domain *ved,
> > vm_event_request_t *req);
> > +    int (*get_response)(struct vm_event_domain *ved,
> > vm_event_response_t *rsp);
> > +    int (*disable)(struct vm_event_domain **_ved);
> 
> I don't see (yet) the reason why having these pointers stored in the
> struct is needed. Are there going to be different implementations for
> these? If so, need to explain that in the commit message.
Yes, this functions will be re-implemented for the
vm_event_domain_channel. I will add an explanation in the commit
message.
> 
> > +
> > +    /* The domain associated with the VM event */
> > +    struct domain *d;
> > +
> > +    /* ring lock */
> > +    spinlock_t lock;
> > +};
> > +
> > +bool vm_event_check(struct vm_event_domain *ved)
> > +{
> > +    return (ved && ved->check(ved));
> > +}
> > 
> > -    return rc;
> > +/* VM event domain ring implementation */
> > +struct vm_event_domain_ring
> > +{
> > +    /* VM event domain */
> > +    struct vm_event_domain ved;
> 
> Why is this not a pointer instead? Does each vm_event_domain_ring
> really need a separate copy of vm_event_domain?

The vm_event_domain structure contains the common attributes for each
domain implementation and it acts more like a "base class".
It must be the first variable in the "derived" structure, so it can be
passed to the implementation specific functions and cast accordingly.
Other than the function pointers, the domain reference and the lock are
separate for each domain.

Also, in order to support legacy applications is better to have the
function interface on a per-domain basis. The only optimization I can
think of is grouping them in a separate "ops" structure:
   struct vm_event_ops ring_ops = {...}
   struct vm_event_ops channel_ops = {...}
and changing the way the functions are called:
   ved->check(ved) ==> ved->ops.check(ved)
Do you favor this approach?

//Petre

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel