[PATCH] grant table map error in __gnttab_map_grant_ref

[PATCH] grant table map error in __gnttab_map_grant_ref

[Liuyongan]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

   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. 
  

Signed-off-by: Haoyu Zhang<haoyu.zhang@huawei.com>; Liang Wang<hzwangliang.wang@huawei.com>

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);




[-- Attachment #2: grant_table.diff --]
[-- Type: application/octet-stream, Size: 926 bytes --]

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);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] grant table map error in __gnttab_map_grant_ref

[Andres Lagar-Cavilla]

> Date: Sat, 04 Feb 2012 02:43:05 +0000
> From: Liuyongan <liuyongan@huawei.com>
> To: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>
> Cc: Wang Liang <hzwangliang.wang@huawei.com>,	Zhanghaoyu
> 	<haoyu.zhang@huawei.com>, Qianhuibin <qianhuibin@huawei.com>,
> 	hanweidong <hanweidong@huawei.com>
> Subject: [Xen-devel] [PATCH] grant table map error in
> 	__gnttab_map_grant_ref
> Message-ID:
> 	<E4ABEE53CC34664FA3F0BD8AEAF50A191CEFCDA6@szxeml534-mbx.china.huawei.com>
>
> Content-Type: text/plain; charset="utf-8"
>
>    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.
>
>
> Signed-off-by: Haoyu Zhang<haoyu.zhang@huawei.com>; Liang
> Wang<hzwangliang.wang@huawei.com>

Hi,
great fix! Would you mind submitting the same fix to the unstable tree?

Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
> 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);
>
>
>
> -------------- next part --------------
> A non-text attachment was scrubbed...
> Name: grant_table.diff
> Type: application/octet-stream
> Size: 926 bytes
> Desc: grant_table.diff
> URL:
> <http://lists.xensource.com/archives/html/xen-devel/attachments/20120204/686972ee/attachment.obj>
>
> ------------------------------
>
> Message: 3
> Date: Fri, 3 Feb 2012 21:57:03 -0500
> From: "Michael A. Collins" <mike.a.collins@ark-net.org>
> To: <xen-devel@lists.xensource.com>
> Subject: [Xen-devel] xen-unstable stubdom build error
> 	version(3432abcf9380)
> Message-ID: <000601cce2e8$af745ab0$0e5d1010$@ark-net.org>
> Content-Type: text/plain;	charset="utf-8"
>
> parent: 24691:3432abcf9380 tip
>  Fix x86_32 build
> branch: default
> commit: 4 modified, 1439 unknown
> update: (current)
>
> /usr/src/xen-4.2/stubdom/ioemu/block-vbd.c: In function ?qemu_aio_wait?:
> /usr/src/xen-4.2/stubdom/ioemu/block-vbd.c:147:20: error: macro
> "remove_waiter" requires 2 arguments, but only 1 given
> /usr/src/xen-4.2/stubdom/ioemu/block-vbd.c:147:5: error: ?remove_waiter?
> undeclared (first use in this function)
> /usr/src/xen-4.2/stubdom/ioemu/block-vbd.c:147:5: note: each undeclared
> identifier is reported only once for each function it appears in
> make[3]: *** [block-vbd.o] Error 1
>
> So I checked some simple stuff out and found this:
> extras/mini-os/fbfront.c:572:    remove_waiter(w, fbfront_queue);
> extras/mini-os/lib/sys.c:237:            remove_waiter(w, console_queue);
> extras/mini-os/lib/sys.c:817:    remove_waiter(netfront_w,
> netfront_queue);
> extras/mini-os/lib/sys.c:818:    remove_waiter(event_w, event_queue);
> extras/mini-os/lib/sys.c:819:    remove_waiter(blkfront_w,
> blkfront_queue);
> extras/mini-os/lib/sys.c:820:    remove_waiter(xenbus_watch_w,
> xenbus_watch_queue);
> extras/mini-os/lib/sys.c:821:    remove_waiter(kbdfront_w,
> kbdfront_queue);
> extras/mini-os/lib/sys.c:822:    remove_waiter(console_w, console_queue);
> extras/mini-os/blkfront.c:326:  remove_waiter(w, blkfront_queue);
> extras/mini-os/blkfront.c:417:    remove_waiter(w, blkfront_queue);
> extras/mini-os/blkfront.c:473:    remove_waiter(w, blkfront_queue);
> extras/mini-os/xenbus/xenbus.c:88:    remove_waiter(w,
> xenbus_watch_queue);
> extras/mini-os/xenbus/xenbus.c:444:    remove_waiter(w,
> req_info[id].waitq);
> extras/mini-os/include/wait.h:60:#define remove_waiter(w, wq) do {  \
> stubdom/ioemu/block-vbd.c:147:    remove_waiter(w);
> tools/qemu-xen-traditional-dir-remote/block-vbd.c:147:
> remove_waiter(w);
> tools/ioemu-remote/block-vbd.c:147:    remove_waiter(w);
> tools/qemu-xen-traditional-dir/block-vbd.c:147:    remove_waiter(w);
> tools/ioemu-dir/block-vbd.c:147:    remove_waiter(w);
>
> It appears to me that possibly I need to either add a queue like the above
> sys.c calls, or define a new macro that does not need a queue?
> Mike
>
>
>
>
> ------------------------------
>
> Message: 4
> Date: Fri, 3 Feb 2012 22:29:25 -0500
> From: "Michael A. Collins" <mike.a.collins@ark-net.org>
> To: <xen-devel@lists.xensource.com>
> Subject: Re: [Xen-devel] xen-unstable stubdom build error
> 	version(3432abcf9380)
> Message-ID: <000b01cce2ed$34bbef40$9e33cdc0$@ark-net.org>
> Content-Type: text/plain;	charset="utf-8"
>
>
>
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-
>> bounces@lists.xensource.com] On Behalf Of Michael A. Collins
>> Sent: Friday, February 03, 2012 9:57 PM
>> To: xen-devel@lists.xensource.com
>> Subject: [Xen-devel] xen-unstable stubdom build error
>> version(3432abcf9380)
>>
>> parent: 24691:3432abcf9380 tip
>>  Fix x86_32 build
>> branch: default
>> commit: 4 modified, 1439 unknown
>> update: (current)
>>
>> /usr/src/xen-4.2/stubdom/ioemu/block-vbd.c: In function
>> ?qemu_aio_wait?:
>> /usr/src/xen-4.2/stubdom/ioemu/block-vbd.c:147:20: error: macro
>> "remove_waiter" requires 2 arguments, but only 1 given
>> /usr/src/xen-4.2/stubdom/ioemu/block-vbd.c:147:5: error:
>> ?remove_waiter? undeclared (first use in this function)
>> /usr/src/xen-4.2/stubdom/ioemu/block-vbd.c:147:5: note: each undeclared
>> identifier is reported only once for each function it appears in
>> make[3]: *** [block-vbd.o] Error 1
>>
>
> I went ahead and cloned into a new directory and all is right with the
> world again.
> Mike
>
>
>
>
> ------------------------------
>
> Message: 5
> Date: Sat, 4 Feb 2012 03:52:40 +0000
> From: xen.org <ian.jackson@eu.citrix.com>
> To: <xen-devel@lists.xensource.com>
> Cc: ian.jackson@eu.citrix.com
> Subject: [Xen-devel] [xen-unstable test] 11868: tolerable FAIL
> Message-ID: <osstest-11868-mainreport@xen.org>
> Content-Type: text/plain
>
> flight 11868 xen-unstable real [real]
> http://www.chiark.greenend.org.uk/~xensrcts/logs/11868/
>
> Failures :-/ but no regressions.
>
> Tests which are failing intermittently (not blocking):
>  test-amd64-amd64-xl-sedf-pin 14 guest-localmigrate/x10      fail pass in
> 11864
>
> Regressions which are regarded as allowable (not blocking):
>  test-i386-i386-win           14 guest-start.2                fail   like
> 11864
>
> Tests which did not succeed, but are not blocking:
>  test-i386-i386-xl-qemuu-winxpsp3  7 windows-install            fail never
> pass
>  test-amd64-amd64-xl-qemuu-winxpsp3  7 windows-install          fail never
> pass
>  test-amd64-amd64-xl-qemuu-win7-amd64  7 windows-install        fail never
> pass
>  test-amd64-i386-qemuu-rhel6hvm-intel  7 redhat-install         fail never
> pass
>  test-amd64-amd64-xl-pcipt-intel  9 guest-start                 fail never
> pass
>  test-amd64-i386-qemuu-rhel6hvm-amd  7 redhat-install           fail never
> pass
>  test-amd64-i386-rhel6hvm-intel 11 leak-check/check             fail never
> pass
>  test-amd64-i386-rhel6hvm-amd 11 leak-check/check             fail   never
> pass
>  test-amd64-amd64-xl-win7-amd64 13 guest-stop                   fail never
> pass
>  test-amd64-i386-xl-winxpsp3-vcpus1 13 guest-stop               fail never
> pass
>  test-amd64-i386-win          16 leak-check/check             fail   never
> pass
>  test-amd64-amd64-xl-win      13 guest-stop                   fail   never
> pass
>  test-i386-i386-xl-win        13 guest-stop                   fail   never
> pass
>  test-amd64-i386-win-vcpus1   16 leak-check/check             fail   never
> pass
>  test-amd64-i386-xend-winxpsp3 16 leak-check/check             fail  never
> pass
>  test-amd64-i386-xl-win7-amd64 13 guest-stop                   fail  never
> pass
>  test-amd64-i386-xl-win-vcpus1 13 guest-stop                   fail  never
> pass
>  test-i386-i386-xl-winxpsp3   13 guest-stop                   fail   never
> pass
>  test-amd64-amd64-win         16 leak-check/check             fail   never
> pass
>  test-amd64-amd64-xl-winxpsp3 13 guest-stop                   fail   never
> pass
>
> version targeted for testing:
>  xen                  3432abcf9380
> baseline version:
>  xen                  3432abcf9380
>
> jobs:
>  build-amd64                                                  pass
>  build-i386                                                   pass
>  build-amd64-oldkern                                          pass
>  build-i386-oldkern                                           pass
>  build-amd64-pvops                                            pass
>  build-i386-pvops                                             pass
>  test-amd64-amd64-xl                                          pass
>  test-amd64-i386-xl                                           pass
>  test-i386-i386-xl                                            pass
>  test-amd64-i386-rhel6hvm-amd                                 fail
>  test-amd64-i386-qemuu-rhel6hvm-amd                           fail
>  test-amd64-amd64-xl-qemuu-win7-amd64                         fail
>  test-amd64-amd64-xl-win7-amd64                               fail
>  test-amd64-i386-xl-win7-amd64                                fail
>  test-amd64-i386-xl-credit2                                   pass
>  test-amd64-amd64-xl-pcipt-intel                              fail
>  test-amd64-i386-rhel6hvm-intel                               fail
>  test-amd64-i386-qemuu-rhel6hvm-intel                         fail
>  test-amd64-i386-xl-multivcpu                                 pass
>  test-amd64-amd64-pair                                        pass
>  test-amd64-i386-pair                                         pass
>  test-i386-i386-pair                                          pass
>  test-amd64-amd64-xl-sedf-pin                                 fail
>  test-amd64-amd64-pv                                          pass
>  test-amd64-i386-pv                                           pass
>  test-i386-i386-pv                                            pass
>  test-amd64-amd64-xl-sedf                                     pass
>  test-amd64-i386-win-vcpus1                                   fail
>  test-amd64-i386-xl-win-vcpus1                                fail
>  test-amd64-i386-xl-winxpsp3-vcpus1                           fail
>  test-amd64-amd64-win                                         fail
>  test-amd64-i386-win                                          fail
>  test-i386-i386-win                                           fail
>  test-amd64-amd64-xl-win                                      fail
>  test-i386-i386-xl-win                                        fail
>  test-amd64-amd64-xl-qemuu-winxpsp3                           fail
>  test-i386-i386-xl-qemuu-winxpsp3                             fail
>  test-amd64-i386-xend-winxpsp3                                fail
>  test-amd64-amd64-xl-winxpsp3                                 fail
>  test-i386-i386-xl-winxpsp3                                   fail
>
>
> ------------------------------------------------------------
> sg-report-flight on woking.cam.xci-test.com
> logs: /home/xc_osstest/logs
> images: /home/xc_osstest/images
>
> Logs, config files, etc. are available at
>     http://www.chiark.greenend.org.uk/~xensrcts/logs
>
> Test harness code can be found at
>     http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary
>
>
> Published tested tree is already up to date.
>
>
>
>
> ------------------------------
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
>
> End of Xen-devel Digest, Vol 84, Issue 82
> *****************************************
>

Re: [PATCH] grant table map error in __gnttab_map_grant_ref

[Jan Beulich]

   >>> On 04.02.12 at 03:43, Liuyongan <liuyongan@huawei.com> 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?

Further, aren't the status flags cumulative (and hence isn't it wrong to
simply clear flags without considering their original state)? (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@huawei.com>; Liang 
> Wang<hzwangliang.wang@huawei.com>
> 
> 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);

Re: [PATCH] grant table map error in __gnttab_map_grant_ref

[Liuyongan]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, February 06, 2012 5:03 PM
> To: Liuyongan
> Cc: hanweidong; Zhanghaoyu; Wang Liang; Qianhuibin; Andres Lagar-
> Cavilla; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] grant table map error in
> __gnttab_map_grant_ref
> 
>    >>> On 04.02.12 at 03:43, Liuyongan <liuyongan@huawei.com> 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.
> 
> 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. 

  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.

> (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@huawei.com>; Liang
> > Wang<hzwangliang.wang@huawei.com>
> >
> > 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);
> 
> 

Re: [PATCH] grant table map error in __gnttab_map_grant_ref

[Liuyongan]

>> Date: Sat, 04 Feb 2012 02:43:05 +0000
>> From: Liuyongan <liuyongan@huawei.com>
>> To: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>
>> Cc: Wang Liang <hzwangliang.wang@huawei.com>,	Zhanghaoyu
>>	<haoyu.zhang@huawei.com>, Qianhuibin <qianhuibin@huawei.com>,
>>	hanweidong <hanweidong@huawei.com>
>> Subject: [Xen-devel] [PATCH] grant table map error in
>>	__gnttab_map_grant_ref
>> Message-ID:
>>	<E4ABEE53CC34664FA3F0BD8AEAF50A191CEFCDA6@szxeml534-mbx.china.huawei.com>
>>
>> Content-Type: text/plain; charset="utf-8"
>>
>>   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.
>> 
>> Signed-off-by: Haoyu Zhang<haoyu.zhang@huawei.com>; Liang
>> Wang<hzwangliang.wang@huawei.com>
>
> Hi,
> great fix! Would you mind submitting the same fix to the unstable tree?

  Feel free to use this patch.
> 
> Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
 
   Well, I donot understand why contents" Message: 3, Message: 4, Message: 5" appear in your initial mail, Any special meaning?

Re: [PATCH] grant table map error in __gnttab_map_grant_ref

[Andres Lagar-Cavilla]

>>> Date: Sat, 04 Feb 2012 02:43:05 +0000
>>> From: Liuyongan <liuyongan@huawei.com>
>>> To: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>
>>> Cc: Wang Liang <hzwangliang.wang@huawei.com>,	Zhanghaoyu
>>>	<haoyu.zhang@huawei.com>, Qianhuibin <qianhuibin@huawei.com>,
>>>	hanweidong <hanweidong@huawei.com>
>>> Subject: [Xen-devel] [PATCH] grant table map error in
>>>	__gnttab_map_grant_ref
>>> Message-ID:
>>>	<E4ABEE53CC34664FA3F0BD8AEAF50A191CEFCDA6@szxeml534-mbx.china.huawei.com>
>>>
>>> Content-Type: text/plain; charset="utf-8"
>>>
>>>   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.
>>>
>>> Signed-off-by: Haoyu Zhang<haoyu.zhang@huawei.com>; Liang
>>> Wang<hzwangliang.wang@huawei.com>
>>
>> Hi,
>> great fix! Would you mind submitting the same fix to the unstable tree?
>
>   Feel free to use this patch.

I certainly will :) But my request is whether you'd be able to rebase it
to the unstable tree, as it seems based on 4.1.1.

>>
>> Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
>    Well, I donot understand why contents" Message: 3, Message: 4, Message:
> 5" appear in your initial mail, Any special meaning?

None at all, just me being a bit lame
Thanks
Andres

>

Re: [PATCH] grant table map error in __gnttab_map_grant_ref

[Jan Beulich]

>>> On 06.02.12 at 12:38, Liuyongan <liuyongan@huawei.com> wrote:

>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Monday, February 06, 2012 5:03 PM
>> To: Liuyongan
>> Cc: hanweidong; Zhanghaoyu; Wang Liang; Qianhuibin; Andres Lagar-
>> Cavilla; xen-devel@lists.xensource.com 
>> Subject: Re: [Xen-devel] [PATCH] grant table map error in
>> __gnttab_map_grant_ref
>> 
>>    >>> On 04.02.12 at 03:43, Liuyongan <liuyongan@huawei.com> 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.
>> 
>> 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).

>   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@huawei.com>; Liang
>> > Wang<hzwangliang.wang@huawei.com>
>> >
>> > 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);
>> 
>> 

Re: [PATCH] grant table map error in __gnttab_map_grant_ref

[Liuyongan]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, February 07, 2012 12:46 AM
> To: Liuyongan
> Cc: hanweidong; Zhanghaoyu; Wang Liang; Qianhuibin; Andres Lagar-
> Cavilla; xen-devel@lists.xensource.com
> Subject: RE: [Xen-devel] [PATCH] grant table map error in
> __gnttab_map_grant_ref
> 
> >>> On 06.02.12 at 12:38, Liuyongan <liuyongan@huawei.com> wrote:
> 
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Monday, February 06, 2012 5:03 PM
> >> To: Liuyongan
> >> Cc: hanweidong; Zhanghaoyu; Wang Liang; Qianhuibin; Andres Lagar-
> >> Cavilla; xen-devel@lists.xensource.com
> >> Subject: Re: [Xen-devel] [PATCH] grant table map error in
> >> __gnttab_map_grant_ref
> >>
> >>    >>> On 04.02.12 at 03:43, Liuyongan <liuyongan@huawei.com> 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@huawei.com>; Liang
> >> > Wang<hzwangliang.wang@huawei.com>
> >> >
> >> > 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);
> >>
> >>
> 
> 

Re: [PATCH] grant table map error in __gnttab_map_grant_ref

[Jan Beulich]

>>> On 07.02.12 at 04:53, Liuyongan <liuyongan@huawei.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >>> On 06.02.12 at 12:38, Liuyongan <liuyongan@huawei.com> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >>    >>> On 04.02.12 at 03:43, Liuyongan <liuyongan@huawei.com> 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().

Ah, okay. Thanks for clarifying that.

>> >> 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.

But still without regard to the previous value these flags had, while
it is my current understanding that these flags provide information
on the kind of uses (note the plural) that a particular grant is in (not
sure what confusion would arise if these flags don't reflect the
actual state, but if the flags getting out of sync was benign, they
could as well be removed).

Jan