Re: [Xen-devel] [PATCH 3/7] vm-event: introduce vm_event_vcpu_enter

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

On 6/17/2016 10:17 AM, Jan Beulich wrote:
> > > > On 16.06.16 at 22:10, <czuzu@xxxxxxxxxxxxxxx> wrote:
> > On 6/16/2016 5:51 PM, Jan Beulich wrote:
> > > > > > On 16.06.16 at 16:08, <czuzu@xxxxxxxxxxxxxxx> wrote:
> > > > @@ -509,6 +508,8 @@ void hvm_do_resume(struct vcpu *v)
> > > >            }
> > > >        }
> > > >    
> > > > +    vm_event_vcpu_enter(v);
> > > Why here?
> > Why indeed. It made sense because monitor_write_data handling was
> > originally there and then the plan was to move it to vm_event_vcpu_enter
> > (which happens in the following commit).
> > The question is though, why was monitor_write_data handled there in the
> > first place? Why was it not put e.g. in vmx_do_resume immediately after
> > the call to hvm_do_resume and just before
> > the reset_stack_and_jump...? And what happens with handling
> > monitor_write_data if this:
> >
> > if ( !handle_hvm_io_completion(v) )
> >           return;
> >
> > causes a return?
> I see Razvan responded to this. I don't have a strong opinion
> either way, my only request if for the call to be in exactly one
> place.
>
> > > > --- a/xen/arch/x86/mm/p2m.c
> > > > +++ b/xen/arch/x86/mm/p2m.c
> > > > @@ -36,7 +36,6 @@
> > > >    #include <asm/hvm/nestedhvm.h>
> > > >    #include <asm/altp2m.h>
> > > >    #include <asm/hvm/svm/amd-iommu-proto.h>
> > > > -#include <asm/vm_event.h>
> > > >    #include <xsm/xsm.h>
> > > There are way too many of these #include adjustments here. If
> > > you really mean to clean these up, please don't randomly throw
> > > this into various unrelated patches.
> > I haven't thrown anything "randomly into unrelated patches", please
> > first ask for my reasoning and then draw such conclusions.
> See patch 1.
"Sorry, that was done out of reflex, should have stated the reasoning."

> Plus I don't think I (or in fact any reviewer) should ask
> for such reasoning: Instead you should state extra cleanup you do
> to unrelated (to the purpose of your patch) files in the description.
Is that still the case when that reasoning is obvious? (at least it 
seemed to me)
but anyway..

> Or even better, split it off to a follow-on, purely cleanup patch.
I agree with this. Will keep in mind with v2.

> (And to be clear, I much appreciate any form of reduction of the
> sometimes extremely long lists of #include-s, just not [apparently
> or really] randomly mixed with other, substantial changes. That's
> namely because it's not clear whether source files should explicitly
> include everything they need, or instead be allowed to rely on
> headers they include to include further headers they also
> _explicitly_ rely on.
Personally I prefer the former since I think it also cuts down 
compilation time.
Having header H include every header Ni needed by source S makes H 
unnecessarily bulky at compilation time for other sources <> S that 
don't need headers Ni but which depend on H nonetheless.
> IOW there's likely a discussion to be had for
> this kind of cleanup, and such a discussion should be a separate
> thread from the one on the functional adjustments here.)
Corneliu.

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