Re: [Xen-devel] [PATCH] grant table map error in __gnttab_map_grant_ref

[Liuyongan <liuyongan@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Tuesday, February 07, 2012 12:46 AM
> To: Liuyongan
> Cc: hanweidong; Zhanghaoyu; Wang Liang; Qianhuibin; Andres Lagar-
> Cavilla; xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: RE: [Xen-devel] [PATCH] grant table map error in
> __gnttab_map_grant_ref
> 
> >>> On 06.02.12 at 12:38, Liuyongan <liuyongan@xxxxxxxxxx> wrote:
> 
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Monday, February 06, 2012 5:03 PM
> >> To: Liuyongan
> >> Cc: hanweidong; Zhanghaoyu; Wang Liang; Qianhuibin; Andres Lagar-
> >> Cavilla; xen-devel@xxxxxxxxxxxxxxxxxxx
> >> Subject: Re: [Xen-devel] [PATCH] grant table map error in
> >> __gnttab_map_grant_ref
> >>
> >>    >>> On 04.02.12 at 03:43, Liuyongan <liuyongan@xxxxxxxxxx> wrote:
> >> > In file grant_table.c function __gnttab_map_grant_ref, if
> >> __get_paged_frame
> >> > failed, the effect of _set_status  previously called should be
> >> rollback, so
> >> > the flag GTF_reading and _GTF_writing will be recovered.
> >>
> >> Not knowing too much about these, but isn't
> __acquire_grant_for_copy()
> >> in need of a similar adjustment?
> >   Well, I need to check it further.

   The caller of __acquire_grant_for_copy() will make sure similar rollback
by calling __release_grant_for_copy().

> >>
> >> Further, aren't the status flags cumulative (and hence isn't it
> wrong
> >> to
> >> simply clear flags without considering their original state)?
> >   When undo_out branch is executed, the flag set by _set_status
> function
> >   will be rolled back by:
> >        if ( !(op->flags & GNTMAP_readonly) &&
> >          !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
> >         gnttab_clear_flag(_GTF_writing, status);
> >
> >        if ( !act->pin )
> >         gnttab_clear_flag(_GTF_reading, status);
> >   so action added by this patch should be correct.
> 
> But this is not a roll-back, here the flags get simply cleared (whereas
> "roll-back" to me means the restoration of their previous value).

  According to my analysis, flag will be cleared only if it is set previously 
in _set_status() of this function.
> 
> >   And anyway, the  reading and writing flag should be recovered when
> mapping
> >   grant reference failed, so the up layer(eg. Netback driver) can
> decide
> >   freely whether retry mapping or fail directly.
> 
> Those flags, afaiu, are managed by Xen, so the layer issuing the
> mapping operation shouldn't be concerned.
> 
> Jan
> 
> >> (I realize
> >> that this is not a new issue introduced with the proposed patch, but
> >> certainly with more ways of getting that code path run through, it
> >> becomes more important for it to be correct.)
> >>
> >> Jan
> >>
> >> > Signed-off-by: Haoyu Zhang<haoyu.zhang@xxxxxxxxxx>; Liang
> >> > Wang<hzwangliang.wang@xxxxxxxxxx>
> >> >
> >> > diff -r cbed91e1c878 xen-4.1.2/xen/common/grant_table.c
> >> > --- a/xen-4.1.2/xen/common/grant_table.c Sat Feb 04 18:36:13
> >> 2012 +0800
> >> > +++ b/xen-4.1.2/xen/common/grant_table.c Sat Feb 04 18:40:02
> >> 2012 +0800
> >> > @@ -566,7 +566,7 @@
> >> >              gfn = sha1 ? sha1->frame : sha2->full_page.frame;
> >> >              rc = __get_paged_frame(gfn, &frame, !!(op->flags &
> >> > GNTMAP_readonly), rd);
> >> >              if ( rc != GNTST_okay )
> >> > -                goto unlock_out;
> >> > +                goto unlock_out_clear;
> >> >              act->gfn = gfn;
> >> >              act->domid = ld->domain_id;
> >> >              act->frame = frame;
> >> > @@ -721,7 +721,8 @@
> >> >      if ( op->flags & GNTMAP_host_map )
> >> >          act->pin -= (op->flags & GNTMAP_readonly) ?
> >> >              GNTPIN_hstr_inc : GNTPIN_hstw_inc;
> >> > -
> >> > +
> >> > + unlock_out_clear:
> >> >      if ( !(op->flags & GNTMAP_readonly) &&
> >> >           !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
> >> >          gnttab_clear_flag(_GTF_writing, status);
> >>
> >>
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel