Re: Multiple patch review

2017-02-16 Thread liuyingdong 
> 发件人: Roger Pau Monné [mailto:roger@citrix.com]
> 发送时间: 2017年2月15日 14:32
> 收件人: Liuyingdong <liuyingd...@huawei.com>
> 抄送: freebsd-xen@freebsd.org;
> 主题: Re: Multiple patch review (was: Re:[PATCH]netfront: need release all 
> resources) after adding and removing NICs time and again
> 
> On Wed, Feb 15, 2017 at 02:06:36PM +0200, liuyingdong wrote:
>> On 2017/2/13 12:05, liuyingdong wrote:
>>> 发件人: roger@citrix.com [mailto:roger@citrix.com]
>>> 发送时间: 2017年2月10日 17:12
>>> 收件人: Liuyingdong <liuyingd...@huawei.com>
>>> 抄送: freebsd-xen@freebsd.org; Suoben <suo...@huawei.com>; Zhaojun
>>> (Euler) <zhao.zhao...@huawei.com>; Wanglinkai
>>> <wanglin...@huawei.com>; chuzhaosong <chuzhaos...@huawei.com>;
>>> Wangchunfeng (Ivan) <chunfeng.w...@huawei.com>; Gaoxiaodong (Leo)
>>> <leo.gaoxiaod...@huawei.com>
>>> 主题: Multiple patch review (was: Re:[PATCH]netfront: need
>>> release all resources) after adding and removing NICs time and again
>>>
>>> On Tue, Feb 07, 2017 at 04:55:34PM +, Liuyingdong wrote:
>>>> Hi Roger,
>>>>  I am so sorry and please review tne below URL:
>>>> https://lists.freebsd.org/pipermail/freebsd-xen/2017-February/002957.html
>>>
>>> Hello,
>>>
>>> Thanks for the patches, and sorry for the delay. It seems like you have not 
>>> applied some of my comments, so I will have to re-post them here. If some 
>>> of the comments don't apply for whatever reason, please reply back and 
>>> explain why, or else this is not going to progress in an useful way for any 
>>> of us.
>>>
>>> I would also request you to look into using `git send-email`, reviewing 
>>> your patches as attachments is not very comfortable. Or else, you could 
>>> create an account to https://reviews.freebsd.org/ and upload the patches 
>>> there assigning me as a reviewer.
>>>
>> I have created an account to https://reviews.freebsd.org/ and uploaded the 
>> patches but I cann't assign you as a reviewer so I assign visible to All 
>> Users.
>> The three modified patches are as follows:
>> 1.https://reviews.freebsd.org/differential/diff/25207/
>> 2.https://reviews.freebsd.org/differential/diff/25208/
>> 3.https://reviews.freebsd.org/differential/diff/25209/
> 
> You need to finish creating the revision, click on "Continue", and add a 
> title/description and reviewers, see 
> https://wiki.freebsd.org/Phabricator#Create_a_Revision_via_Web_Interface for 
> more information.
> 
Thank you for the time and patience you devoted to guide me how to create a 
revision. I have created three revisions and please review again.
https://reviews.freebsd.org/D9635
https://reviews.freebsd.org/D9638
https://reviews.freebsd.org/D9639
> Roger.
> 

Thanks, Terry.

___
freebsd-xen@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-xen
To unsubscribe, send any mail to "freebsd-xen-unsubscr...@freebsd.org"

Re: Multiple patch review (was: Re: 答复: 答复: [PATCH]netfront: need release all resources) after adding and removing NICs time and again

2017-02-15 Thread Roger Pau Monné 
On Wed, Feb 15, 2017 at 02:06:36PM +0200, liuyingdong wrote:
> On 2017/2/13 12:05, liuyingdong wrote:
> > 发件人: roger@citrix.com [mailto:roger@citrix.com]
> > 发送时间: 2017年2月10日 17:12
> > 收件人: Liuyingdong 
> > 抄送: freebsd-xen@freebsd.org; Suoben ; Zhaojun (Euler) 
> > ; Wanglinkai ; chuzhaosong 
> > ; Wangchunfeng (Ivan) ; 
> > Gaoxiaodong (Leo) 
> > 主题: Multiple patch review (was: Re: 答复: 答复: [PATCH]netfront: need release 
> > all resources) after adding and removing NICs time and again
> > 
> > On Tue, Feb 07, 2017 at 04:55:34PM +, Liuyingdong wrote:
> >> Hi Roger,
> >>  I am so sorry and please review tne below URL:
> >> https://lists.freebsd.org/pipermail/freebsd-xen/2017-February/002957.h
> >> tml
> > 
> > Hello,
> > 
> > Thanks for the patches, and sorry for the delay. It seems like you have not 
> > applied some of my comments, so I will have to re-post them here. If some 
> > of the comments don't apply for whatever reason, please reply back and 
> > explain why, or else this is not going to progress in an useful way for any 
> > of us.
> > 
> > I would also request you to look into using `git send-email`, reviewing 
> > your patches as attachments is not very comfortable. Or else, you could 
> > create an account to https://reviews.freebsd.org/ and upload the patches 
> > there assigning me as a reviewer.
> > 
> I have created an account to https://reviews.freebsd.org/ and uploaded the 
> patches but I cann't assign you as a reviewer so I assign visible to All 
> Users.
> The three modified patches are as follows:
> 1.https://reviews.freebsd.org/differential/diff/25207/
> 2.https://reviews.freebsd.org/differential/diff/25208/
> 3.https://reviews.freebsd.org/differential/diff/25209/

You need to finish creating the revision, click on "Continue", and add a
title/description and reviewers, see
https://wiki.freebsd.org/Phabricator#Create_a_Revision_via_Web_Interface for
more information.

Roger.
___
freebsd-xen@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-xen
To unsubscribe, send any mail to "freebsd-xen-unsubscr...@freebsd.org"

Re: Multiple patch review (was: Re: 答复: 答复: [PATCH]netfront: need release all resources) after adding and removing NICs time and again

2017-02-15 Thread liuyingdong 
On 2017/2/13 12:05, liuyingdong wrote:
> 发件人: roger@citrix.com [mailto:roger@citrix.com]
> 发送时间: 2017年2月10日 17:12
> 收件人: Liuyingdong 
> 抄送: freebsd-xen@freebsd.org; Suoben ; Zhaojun (Euler) 
> ; Wanglinkai ; chuzhaosong 
> ; Wangchunfeng (Ivan) ; 
> Gaoxiaodong (Leo) 
> 主题: Multiple patch review (was: Re: 答复: 答复: [PATCH]netfront: need release all 
> resources) after adding and removing NICs time and again
> 
> On Tue, Feb 07, 2017 at 04:55:34PM +, Liuyingdong wrote:
>> Hi Roger,
>>  I am so sorry and please review tne below URL:
>> https://lists.freebsd.org/pipermail/freebsd-xen/2017-February/002957.h
>> tml
> 
> Hello,
> 
> Thanks for the patches, and sorry for the delay. It seems like you have not 
> applied some of my comments, so I will have to re-post them here. If some of 
> the comments don't apply for whatever reason, please reply back and explain 
> why, or else this is not going to progress in an useful way for any of us.
> 
> I would also request you to look into using `git send-email`, reviewing your 
> patches as attachments is not very comfortable. Or else, you could create an 
> account to https://reviews.freebsd.org/ and upload the patches there 
> assigning me as a reviewer.
> 
I have created an account to https://reviews.freebsd.org/ and uploaded the 
patches but I cann't assign you as a reviewer so I assign visible to All Users.
The three modified patches are as follows:
1.https://reviews.freebsd.org/differential/diff/25207/
2.https://reviews.freebsd.org/differential/diff/25208/
3.https://reviews.freebsd.org/differential/diff/25209/
> When replying, can you also make sure that you reply in-line to my comments, 
> or else it's very hard to follow the conversation.
> 
ok
> ---
>> From 0af05ac01be853340b3b53862de9853d8d44c0c6 Mon Sep 17 00:00:00 2001
>> From: liuyingdong 
>> Date: Thu, 2 Feb 2017 19:40:48 +0200
>> Subject: introduce suspend_cancel mechanism for frontend devices.
>> 1.1 On a cancelled suspend, xen frontend devices need know their state is 
>> invariant.
>> 1.2 On a cancelled suspend the vcpu_info location does not change
>> (it's still in the per-cpu area registered by xen_hvm_cpu_init()).
>> So do not call xen_hvm_init_shared_info_page() which would make the kernel 
>> think its back in the shared info.
>> With the wrong vcpu_info, events cannot be received and the domain will hang 
>> after a cancelled suspend.
>>
>> ---
>>  dev/xen/blkfront/blkfront.c |  5 +
>>  dev/xen/control/control.c   | 11 +--
>>  dev/xen/netfront/netfront.c | 21 +
>>  x86/xen/hvm.c   | 16 ++--
>>  xen/xenbus/xenbusb.c| 35 +++
>>  xen/xenbus/xenbusvar.h  |  2 ++
>>  6 files changed, 66 insertions(+), 24 deletions(-)
>>
>> diff --git a/dev/xen/blkfront/blkfront.c b/dev/xen/blkfront/blkfront.c
>> index 9eca220..47cd83f 100644
>> --- a/dev/xen/blkfront/blkfront.c
>> +++ b/dev/xen/blkfront/blkfront.c
>> @@ -1537,6 +1537,11 @@ xbd_resume(device_t dev)  {
>>  struct xbd_softc *sc = device_get_softc(dev);
>>
>> +if(g_suspend_cancelled) {
>> +sc->xbd_state = XBD_STATE_CONNECTED;
>> +return (0);
>> +}
>> +
>>  DPRINTK("xbd_resume: %s\n", xenbus_get_node(dev));
>>
>>  xbd_free(sc);
>> diff --git a/dev/xen/control/control.c b/dev/xen/control/control.c
>> index 58fefcc..4f8471f 100644
>> --- a/dev/xen/control/control.c
>> +++ b/dev/xen/control/control.c
>> @@ -190,6 +190,11 @@ xctrl_reboot()
>>  shutdown_nice(0);
>>  }
>>
>> +static void _set_suspend_cancelled(bool suspend_cancelled) {
>> +g_suspend_cancelled = suspend_cancelled; }
>> +
>>  static void
>>  xctrl_suspend()
>>  {
>> @@ -278,7 +283,9 @@ xctrl_suspend()
>>  /*
>>   * Reset grant table info.
>>   */
>> -gnttab_resume(NULL);
>> +if(suspend_cancelled == 0)  {
>> +gnttab_resume(NULL);
>> +}
>>
>>  #ifdef SMP
>>  if (!CPU_EMPTY(_suspend_map)) {
>> @@ -296,6 +303,7 @@ xctrl_suspend()
>>   * FreeBSD really needs to add DEVICE_SUSPEND_CANCEL or
>>   * similar.
>>   */
>> +_set_suspend_cancelled(suspend_cancelled != 0);
> 
> No need for this helper, you can just s/suspend_cancelled/xen_suspend_called/
> and make it global.
> 
ok, I agree with you.
>>  DEVICE_RESUME(root_bus);
>>  mtx_unlock();
>>
>> @@ -319,7 +327,6 @@ xctrl_suspend()
>>  #endif
>>
>>  resume_all_proc();
>> -
> 
> Stray change.
> 
ok, I agree with you.
>>  EVENTHANDLER_INVOKE(power_resume);
>>
>>  if (bootverbose)
>> diff --git a/dev/xen/netfront/netfront.c b/dev/xen/netfront/netfront.c
>> index 459712a..537018a 100644
>> --- a/dev/xen/netfront/netfront.c
>> +++ b/dev/xen/netfront/netfront.c
>> @@ -153,6 +153,10 @@ static int xn_get_responses(struct