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); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com lists.xensource.com/xen-devel
Andres Lagar-Cavilla
2012-Feb-04 04:58 UTC
Re: [PATCH] grant table map error in __gnttab_map_grant_ref
> 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: > <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] > 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 > chiark.greenend.org.uk/~xensrcts/logs > > Test harness code can be found at > 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 > lists.xensource.com/xen-devel > > > End of Xen-devel Digest, Vol 84, Issue 82 > ***************************************** >
Jan Beulich
2012-Feb-06 09:03 UTC
Re: [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? 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);
Liuyongan
2012-Feb-06 11:38 UTC
Re: [PATCH] grant table map error in __gnttab_map_grant_ref
> -----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); > >
Liuyongan
2012-Feb-06 12:20 UTC
Re: [PATCH] grant table map error in __gnttab_map_grant_ref
>> 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?
Andres Lagar-Cavilla
2012-Feb-06 15:45 UTC
Re: [PATCH] grant table map error in __gnttab_map_grant_ref
>>> 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>
Jan Beulich
2012-Feb-06 16:45 UTC
Re: [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. >> >> 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); >> >>
Liuyongan
2012-Feb-07 03:53 UTC
Re: [PATCH] grant table map error in __gnttab_map_grant_ref
> -----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); > >> > >> > >
Jan Beulich
2012-Feb-07 08:36 UTC
Re: [PATCH] grant table map error in __gnttab_map_grant_ref
>>> 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