Re: [Xen-devel] [PATCH V5 07/12] xen: Introduce monitor_op domctl

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

On Wed Feb 18 2015 10:26:00 AM CET, Jan Beulich <JBeulich@xxxxxxxx> wrote:

> > > > On 17.02.15 at 19:20, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> > On Tue, Feb 17, 2015 at 3:02 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> > > > > > On 13.02.15 at 17:33, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> > > > rc = vm_event_enable(d, vec, ved, _VPF_mem_access,
> > > > -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  
> > > > HVM_PARAM_MONITOR_RING_PFN,
> > > > -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  
> > > > mem_access_notification);
> > > > -
> > > > -Â Â Â Â Â Â Â Â Â Â Â  if ( vec->op ==
> > > > XEN_VM_EVENT_MONITOR_ENABLE_INTROSPECTION -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â
> > > >  Â  && !rc
> > > > ) -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  p2m_setup_introspection(d);
> > > > -
> > > > -Â Â Â Â Â Â Â  }
> > > > -Â Â Â Â Â Â Â  break;
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  
> > > > HVM_PARAM_MONITOR_RING_PFN,
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  
> > > > mem_access_notification);
> > > 
> > > I don't see what changes for these two lines. If it's indentation, it
> > > should be done right when the code gets added.
> > 
> > Indentation can't be fixed in the code addition as it breaks git -M.
> > It reverts to the old format where it just removes the whole file and
> > adds the new one. I think its a waste to add a whole new separate
> > patch just to fix indentations so I just fix it here.
> 
> Considering that indentation is broken already prior to your
> series, this is perhaps acceptable. But at least if indentation
> was correct before the rename, it should be afterwards. You'd
> have to use of git's -B option to control the resulting diff.
> 
> > > > +#include <public/domctl.h>
> > > > +
> > > > +static inline
> > > > +int monitor_domctl(struct xen_domctl_monitor_op *op, struct
> > > > domain *d)
> > > 
> > > The includes above are insufficient for the types used, or you should
> > > forward declare _both_ structs and not have any includes.
> > 
> > Just including sched.h additionally should be enough IMHO.
> 
> Resulting in a huge pile of further dependencies. Our goal really
> should be to get the dependencies down, not up - improving build
> time. Hence forward declarations are very likely the better choice
> here.
> 
> > > > --- a/xen/include/asm-x86/domain.h
> > > > +++ b/xen/include/asm-x86/domain.h
> > > > @@ -241,6 +241,24 @@ struct time_scale {
> > > > u32 mul_frac;
> > > > };
> > > > 
> > > > +/************************************************/
> > > > +/*            monitor event options             
> > > > */
> > > > +/************************************************/
> > > > +struct mov_to_cr {
> > > > +Â Â Â  uint8_t enabled;
> > > > +Â Â Â  uint8_t sync;
> > > > +Â Â Â  uint8_t onchangeonly;
> > > > +};
> > > > +
> > > > +struct mov_to_msr {
> > > > +Â Â Â  uint8_t enabled;
> > > > +Â Â Â  uint8_t extended_capture;
> > > > +};
> > > > +
> > > > +struct debug_event {
> > > > +Â Â Â  uint8_t enabled;
> > > > +};
> > > 
> > > These are all internal structures - is there anything wrong with
> > > using bitfields here?
> > 
> > The use if bitfields is not good performance-wise AFAIK. Would there
> > be any benefit that would offset that?
> 
> As Andrew already said - total structure size. Also I'm pretty
> convinced "or $<val>, <mem>" as well as "and $~<val>,<mem>"
> aren't much worse than "mov $<val>,<mem>", and the code
> writing these fields shouldn't be performance critical. And
> "test $<val>,<mem>" and "cmp $<val>,<mem>" (as well as
> their split up alternatives, should the compiler elect to do so)
> ought to be equal performance wise.
> 
> Jan

OK, I'll switch to bitfields and adjust the patch accordingly.

Tamas


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel