Re: [Xen-devel] [xen-unstable test] 104131: regressions - FAIL

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, January 16, 2017 7:00 PM
> 
> >>> On 16.01.17 at 06:25, <kevin.tian@xxxxxxxxx> wrote:
> > One thing noted though. The original patch from Quan is actually orthogonal
> > to this ASSERT. Regardless of whether intack.vector is larger or smaller
> > than pt_vector, we always require the trick as long as pt_vector is not the
> > one being currently programmed to RVI.
> 
> I don't think the ASSERT() addition is orthogonal: It exchanges
> intack.vector for pt_vector in the invocation of
> vmx_set_eoi_exit_bitmap(), and during discussion of the patch
> there at least intermediately was max() of the two used instead.
> It was - iirc - one of you who suggested that the use of max()
> there is unnecessary, which the ASSERT() triggering has now
> shown is wrong.

Attached was my earlier comment:

--
> >>> On 20.12.16 at 06:37, <kevin.tian@xxxxxxxxx> wrote:
> >>  From: Xuquan (Quan Xu) [mailto:xuquan8@xxxxxxxxxx]
> >> Sent: Friday, December 16, 2016 5:40 PM
> >> -        if (pt_vector != -1)
> >> -            vmx_set_eoi_exit_bitmap(v, pt_vector);
> >> +        if ( pt_vector != -1 ) {
> >> +            if ( intack.vector > pt_vector )
> >> +                vmx_set_eoi_exit_bitmap(v, intack.vector);
> >> +            else
> >> +                vmx_set_eoi_exit_bitmap(v, pt_vector);
> >> +        }
> >
> > Above can be simplified as one line change:
> >     if ( pt_vector != -1 )
> >             vmx_set_eoi_exit_bitmap(v, intack.vector);
> 
> Hmm, I don't understand. Did you mean to use max() here? Or
> else how is this an equivalent of the originally proposed code?
> 

Original code is not 100% correct. The purpose is to set EOI exit
bitmap for any vector which may block injection of pt_vector - 
give chance to recognize pt_vector in future intack and then do pt 
intr post. The simplified code achieves this effect same as original
code if intack.vector >= vector. I cannot come up a case why
intack.vector might be smaller than vector. If this case happens,
we still need enable exit bitmap for intack.vector instead of
pt_vector for said purpose while original code did it wrong.

Thanks
Kevin
--

Using intack.vector is always expected here regardless of the 
comparison result between intack.vector and pt_vector. The 
reason why I was OK adding an ASSERT was simply to test 
whether intack.vecor<pt_vector does happen which is
orthogonal to the fix itself.

> 
> > Then do we want to revert the whole
> > commit until the problem is finally fixed, or OK to just remove ASSERT
> > (or replace with WARN_ON with more debug info) to unblock test system
> > before the fix is ready?
> 
> Well, as the VMX maintainer I think the proposal of whether to
> revert or wait should really come from you.
> 
> Jan

Andrew, how long do you usually tolerate a failure case in osstest?
I'm not sure how long it may take for developer to reproduce this
situation. If it has blocking impact in your side, I'd suggest go 
replacing ASSERT with more informative warn info before final 
root-cause, if Quan&Chao cannot reproduce in a short time (say
1 or 2wk or so).

Thanks
Kevin

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