* Re: [PATCH] grant table map error in __gnttab_map_grant_ref
@ 2012-02-06 12:20 Liuyongan
2012-02-06 15:45 ` Andres Lagar-Cavilla
0 siblings, 1 reply; 9+ messages in thread
From: Liuyongan @ 2012-02-06 12:20 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: Zhanghaoyu, xen-devel@lists.xensource.com, Qianhuibin,
Jan Beulich, hanweidong
>> 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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] grant table map error in __gnttab_map_grant_ref
2012-02-06 12:20 [PATCH] grant table map error in __gnttab_map_grant_ref Liuyongan
@ 2012-02-06 15:45 ` Andres Lagar-Cavilla
0 siblings, 0 replies; 9+ messages in thread
From: Andres Lagar-Cavilla @ 2012-02-06 15:45 UTC (permalink / raw)
To: Liuyongan
Cc: Zhanghaoyu, xen-devel@lists.xensource.com, Qianhuibin,
Jan Beulich, hanweidong
>>> 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <mailman.2630.1328327569.1471.xen-devel@lists.xensource.com>]
* Re: [PATCH] grant table map error in __gnttab_map_grant_ref
[not found] <mailman.2630.1328327569.1471.xen-devel@lists.xensource.com>
@ 2012-02-04 4:58 ` Andres Lagar-Cavilla
0 siblings, 0 replies; 9+ messages in thread
From: Andres Lagar-Cavilla @ 2012-02-04 4:58 UTC (permalink / raw)
To: xen-devel
> 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
> *****************************************
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] grant table map error in __gnttab_map_grant_ref
@ 2012-02-04 2:43 Liuyongan
2012-02-06 9:03 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Liuyongan @ 2012-02-04 2:43 UTC (permalink / raw)
To: xen-devel@lists.xensource.com
Cc: Wang Liang, Zhanghaoyu, Qianhuibin, hanweidong
[-- 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] grant table map error in __gnttab_map_grant_ref
2012-02-04 2:43 Liuyongan
@ 2012-02-06 9:03 ` Jan Beulich
2012-02-06 11:38 ` Liuyongan
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2012-02-06 9:03 UTC (permalink / raw)
To: Liuyongan
Cc: xen-devel@lists.xensource.com, Qianhuibin, hanweidong, Zhanghaoyu,
Andres Lagar-Cavilla, Wang Liang
>>> 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);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] grant table map error in __gnttab_map_grant_ref
2012-02-06 9:03 ` Jan Beulich
@ 2012-02-06 11:38 ` Liuyongan
2012-02-06 16:45 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Liuyongan @ 2012-02-06 11:38 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel@lists.xensource.com, Qianhuibin, hanweidong, Zhanghaoyu,
Andres Lagar-Cavilla, Wang Liang
> -----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);
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] grant table map error in __gnttab_map_grant_ref
2012-02-06 11:38 ` Liuyongan
@ 2012-02-06 16:45 ` Jan Beulich
2012-02-07 3:53 ` Liuyongan
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2012-02-06 16:45 UTC (permalink / raw)
To: Liuyongan
Cc: xen-devel@lists.xensource.com, Qianhuibin, hanweidong, Zhanghaoyu,
Andres Lagar-Cavilla, Wang Liang
>>> 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);
>>
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] grant table map error in __gnttab_map_grant_ref
2012-02-06 16:45 ` Jan Beulich
@ 2012-02-07 3:53 ` Liuyongan
2012-02-07 8:36 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Liuyongan @ 2012-02-07 3:53 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel@lists.xensource.com, Qianhuibin, hanweidong, Zhanghaoyu,
Andres Lagar-Cavilla, Wang Liang
> -----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);
> >>
> >>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] grant table map error in __gnttab_map_grant_ref
2012-02-07 3:53 ` Liuyongan
@ 2012-02-07 8:36 ` Jan Beulich
0 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2012-02-07 8:36 UTC (permalink / raw)
To: Liuyongan
Cc: xen-devel@lists.xensource.com, Qianhuibin, hanweidong, Zhanghaoyu,
Andres Lagar-Cavilla, Wang Liang
>>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-02-07 8:36 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-02-06 12:20 [PATCH] grant table map error in __gnttab_map_grant_ref Liuyongan
2012-02-06 15:45 ` Andres Lagar-Cavilla
[not found] <mailman.2630.1328327569.1471.xen-devel@lists.xensource.com>
2012-02-04 4:58 ` Andres Lagar-Cavilla
-- strict thread matches above, loose matches on Subject: below --
2012-02-04 2:43 Liuyongan
2012-02-06 9:03 ` Jan Beulich
2012-02-06 11:38 ` Liuyongan
2012-02-06 16:45 ` Jan Beulich
2012-02-07 3:53 ` Liuyongan
2012-02-07 8:36 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).