From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v2 1/2] VT-d: Reduce spin timeout to 1ms, which can be boot-time changed. Date: Mon, 14 Dec 2015 01:49:53 -0700 Message-ID: <566E90C102000078000BEFBA@prv-mh.provo.novell.com> References: <1449739990-66155-1-git-send-email-quan.xu@intel.com> <1449739990-66155-2-git-send-email-quan.xu@intel.com> <566AAD0902000078000BE8C5@prv-mh.provo.novell.com> <945CA011AD5F084CBEA3E851C0AB28894AE59E11@SHSMSX103.ccr.corp.intel.com> <566E897702000078000BEF62@prv-mh.provo.novell.com> <945CA011AD5F084CBEA3E851C0AB28894AE5A350@SHSMSX103.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <945CA011AD5F084CBEA3E851C0AB28894AE5A350@SHSMSX103.ccr.corp.intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Quan Xu Cc: "tim@xen.org" , Kevin Tian , Feng Wu , "george.dunlap@eu.citrix.com" , "andrew.cooper3@citrix.com" , Eddie Dong , "xen-devel@lists.xen.org" , Jun Nakajima , "keir@xen.org" List-Id: xen-devel@lists.xenproject.org >>> On 14.12.15 at 09:31, wrote: >> On 14.12.2015 at 4:19pm, wrote: >> >>> On 12.12.15 at 10:03, wrote: >> >> On 11.12.2015 at 6:01pm, wrpte: >> >> >>> On 10.12.15 at 10:33, wrote: >> >> > @@ -167,10 +172,12 @@ static int queue_invalidate_wait(struct iommu >> >> *iommu, >> >> > start_time = NOW(); >> >> > while ( poll_slot != QINVAL_STAT_DONE ) >> >> > { >> >> > - if ( NOW() > (start_time + DMAR_OPERATION_TIMEOUT) ) >> >> > + if ( NOW() > (start_time + IOMMU_QI_TIMEOUT) ) >> >> > { >> >> > print_qi_regs(iommu); >> >> > - panic("queue invalidate wait descriptor was not >> >> executed"); >> >> > + dprintk(XENLOG_WARNING VTDPREFIX, >> >> > + "Queue invalidate wait descriptor was >> >> timeout.\n"); >> >> > + return -ETIMEDOUT; >> >> > } >> >> >> >> I don't see such a change be valid without making sure callers >> >> actually honor errors. For example, no caller of >> >> iommu_flush_iec_{global,index}() cares to check. And not even your second >> patch addresses this (i.e. >> >> it's also not just bad patch ordering). >> >> >> > >> > I check it again. >> > For iommu_flush_iec_{global,index}() are both call __iommu_flush_iec(). >> > In my patch, I have check it in __iommu_flush_iec(). >> > I think it does not need to check it in >> > iommu_flush_iec_{global,index}() again. Right? >> >> No, not in the v2 version of it. While you call invalidate_timeout() in the >> -ETIMEDOUT case, you still pass on the error code, and hence the callers need >> to also either pass it on or deal with it. > > Jan, sorry for that. I still don't get the point. > Check it again :(:( > > Should I also check it where call iommu_flush_iec_{global,index}(), if it is > -ETIMEDOUT, it should return or deal with it?? You should check for _any_ kind of error here. I.e. the problem doesn't get introduced by your patches, it only gets made worse (by virtue of the fact that the only error queue_invalidate_wait() may return before your patch is for when calling code sets a flag it isn't currently supposed to set, i.e. not handling errors right now is pretty much benign), and hence properly dealing with errors here would probably best go into a prereq patch. Jan