On March 18, 2016 4:10pm, <jbeul...@suse.com> wrote:
> >>> On 18.03.16 at 04:19, <quan...@intel.com> wrote:
> > On March 17, 2016 8:34pm, George Dunlap <george.dun...@eu.citrix.com>
> wrote:
> >> On Thu, Mar 17, 2016 at 12:30 PM, George Dunlap
> >> <george.dun...@eu.citrix.com> wrote:
> >> > On Thu, Mar 17, 2016 at 6:54 AM, Quan Xu <quan...@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
