Re: [PATCH 1/2] IOMMU/MMU: Adjust top level functions for VT-d Device-TLB flush error.

["Xu, Quan" <quan.xu@intel.com>]

On March 18, 2016 4:10pm, <JBeulich@suse.com> wrote:
> >>> On 18.03.16 at 04:19, <quan.xu@intel.com> wrote:
> > On March 17, 2016 8:34pm, George Dunlap <George.Dunlap@eu.citrix.com>
> wrote:
> >> On Thu, Mar 17, 2016 at 12:30 PM, George Dunlap
> >> <George.Dunlap@eu.citrix.com> wrote:
> >> > On Thu, Mar 17, 2016 at 6:54 AM, Quan Xu <quan.xu@intel.com> wrote:
> >> >> --- a/xen/arch/x86/mm/p2m-ept.c
> >> >> +++ b/xen/arch/x86/mm/p2m-ept.c
> >> >> @@ -830,7 +830,15 @@ out:
> >> >>          {
> >> >>              if ( iommu_flags )
> >> >>                  for ( i = 0; i < (1 << order); i++ )
> >> >> -                    iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
> >> iommu_flags);
> >> >> +                {
> >> >> +                    rc = iommu_map_page(d, gfn + i, mfn_x(mfn) +
> >> >> + i,
> >> iommu_flags);
> >> >> +                    if ( rc )
> >> >> +                    {
> >> >> +                        while ( i-- > 0 )
> >> >> +                            iommu_unmap_page(d, gfn + i);
> >> >
> >> > This won't unmap gfn+0 (since it will break out when i == 0 without
> >> > calling unmap).
> >>
> >> Oh, no it won't, because the decrement is postfix.
> >>
> >> For us mere mortals, I'd appreciate a comment here like this:
> >>
> >> /* Postfix operator means we will call unmap with i == 0 */
> >>
> > Agreed.
> > For these 2 points, to summarize:
> >    - adding "unlikely()" to the if() condition, e.g. if ( unlikely(rc) )
> >    - adding a comment:
> >         /* Postfix operator means we will call unmap with i == 0 */
> 
> To be honest, I'm opposed to the addition of such comments.
> See also the parallel discussion rooted at
> http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg01779.html
> 

Reading the Follow-Ups email, it looks a pretty common cleanup pattern.
now I don't fully get this point, but I would try to follow this pattern.

Quan

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