Re: [PATCH]netfront: need release all resources after adding and removing NICs time and again

[Liuyingdong]

Hello Roger,
Thank you for the time and patience you devoted to reading my messages 
and e-mails. I really appreciate that.
Depend on your reply, I submit those against current HEAD 
(origin/master).

1. 0001 patch: 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.

I declare a global suspend_cancelled variable in xen/xenbus/xenbusvar.h and 
export that to all the frontends that need it. It should also be a bool type 
instead of int. I merge together with a _set_suspend_cancelled() function.

2. 0002 patch: a userspace application need wait until stop_all_proc has 
finished during live migration.
If there is a user process which maybe often reads and writes xenstore, the 
application has been suspended after stop_all_proc is called. It held 
xs.request_mutex lock but in the xctrl_suspend() function xs_write and 
xs_suspend will not obtain this lock. So the VM will hang. In order to prevent 
this from happening, this process need wait until stop_all_proc has finished 
during live migration.

3. 0003 patch: wrong order of device resume causes virtual machine to become 
unresponsive after live migration.
Because wrong order of device resume, VM will hang after live migration. attach 
the Xen PV timer to the nexus directly as it was done before.

In my laboratory, FreeBSD VMs under no load and high load(cpu 80% and 
memory 80%) were alive after made the live migration back and forth 1000 times.
I want to know what about these patch, Please let me know if you have 
any questions. Thanks!

-邮件原件-
发件人: freebsd xen [mailto:roger@citrix.com] 
发送时间: 2017年1月6日 14:44
收件人: Liuyingdong <liuyingd...@huawei.com>
抄送: freebsd-xen@freebsd.org; Zhaojun (Euler) <zhao.zhao...@huawei.com>; Suoben 
<suo...@huawei.com>; Ouyangzhaowei (Charles) <ouyangzhao...@huawei.com>; 
chuzhaosong <chuzhaos...@huawei.com>; Wanglinkai <wanglin...@huawei.com>
主题: Re: [PATCH]netfront: need release all resources after adding and removing 
NICs time and again

On Thu, Dec 15, 2016 at 02:00:53AM +, Liuyingdong wrote:
> Hello Roger,
>   Thank you for the time and patience you devoted to reading my messages 
> and e-mails. I really appreciate that.
>   I can't use git send-email so I attach the patches directly. In the 
> 0001 patch I introduce suspend_cancel mechanism for frontend devices and in 
> the 0002 patch I release all resources after hot unplug NICs.
> 
> Note: These two patches is on the base of the origin/release/10.2.0 
> branch and the 0002 patch is made after the 0001 patch.

Hello,

Thanks for the patches, now they look fine. You will however need to submit 
those against current HEAD (origin/master), and then I will do the backport to
stable/10 and stable/11. I cannot apply a patch directly to stable branches 
without it being in HEAD first unless there's a good reason for it.

Replying to both patches here inline.

---8<---
> From fc85ac7eba55a5f14f5f7d81f0e1fc7fbf6a7447 Mon Sep 17 00:00:00 2001
> From: Yingdong Liu <liuyingd...@huawei.com>
> Date: Tue, 13 Dec 2016 21:53:25 +0800
> Subject: [PATCH] introduce suspend cancel mechanism for frontend 
> devices
> 

Can you elaborate a little bit more on the commit message here? What issues are 
you seeing without this patch applied? What is the result after applying the 
patch?

Commit messages are very important in order to know why a change is needed, 
specially when you look at them in for example 3 years time.

> ---
>  sys/dev/xen/blkfront/blkfront.c | 13 +++
>  sys/dev/xen/control/control.c   |  9 ++-
>  sys/dev/xen/netfront/netfront.c | 52 
> ++---
>  sys/x86/xen/hvm.c   | 16 -
>  sys/xen/xenbus/xenbusb.c| 45 ++-
>  sys/xen/xenbus/xenbusvar.h  |  4 
>  6 files changed, 102 insertions(+), 37 deletions(-)
> 
> diff --git a/sys/dev/xen/blkfront/blkfront.c 
> b/sys/dev/xen/blkfront/blkfront.c index a71251d..8d7c32a 100644
> --- a/sys/dev/xen/blkfront/blkfront.c
> +++ b/sys/dev/xen/blkfront/blkfront.c
> @@ -68,6 +68,8 @@ __FBSDID("$FreeBSD$");
>  
>  #include "xenbus_if.h"
>  
> +static int blkfront_suspend_cancelled = 0;

I see that you add one of those to each frontend you add cancel suspend 
support, together with a *_set_suspend_cancel function. Wouldn't it be easier 
to declare a global suspend_c

Re: [PATCH]netfront: need release all resources after adding and removing NICs time and again

[freebsd xen]

On Thu, Dec 15, 2016 at 02:00:53AM +, Liuyingdong wrote:
> Hello Roger,
>   Thank you for the time and patience you devoted to reading my messages 
> and e-mails. I really appreciate that.
>   I can't use git send-email so I attach the patches directly. In the 
> 0001 patch I introduce suspend_cancel mechanism for frontend devices and in 
> the 0002 patch I release all resources after hot unplug NICs.
> 
> Note: These two patches is on the base of the origin/release/10.2.0 
> branch and the 0002 patch is made after the 0001 patch.

Hello,

Thanks for the patches, now they look fine. You will however need to submit
those against current HEAD (origin/master), and then I will do the backport to
stable/10 and stable/11. I cannot apply a patch directly to stable branches
without it being in HEAD first unless there's a good reason for it.

Replying to both patches here inline.

---8<---
> From fc85ac7eba55a5f14f5f7d81f0e1fc7fbf6a7447 Mon Sep 17 00:00:00 2001
> From: Yingdong Liu 
> Date: Tue, 13 Dec 2016 21:53:25 +0800
> Subject: [PATCH] introduce suspend cancel mechanism for frontend devices
> 

Can you elaborate a little bit more on the commit message here? What issues are
you seeing without this patch applied? What is the result after applying the
patch?

Commit messages are very important in order to know why a change is needed,
specially when you look at them in for example 3 years time.

> ---
>  sys/dev/xen/blkfront/blkfront.c | 13 +++
>  sys/dev/xen/control/control.c   |  9 ++-
>  sys/dev/xen/netfront/netfront.c | 52 
> ++---
>  sys/x86/xen/hvm.c   | 16 -
>  sys/xen/xenbus/xenbusb.c| 45 ++-
>  sys/xen/xenbus/xenbusvar.h  |  4 
>  6 files changed, 102 insertions(+), 37 deletions(-)
> 
> diff --git a/sys/dev/xen/blkfront/blkfront.c b/sys/dev/xen/blkfront/blkfront.c
> index a71251d..8d7c32a 100644
> --- a/sys/dev/xen/blkfront/blkfront.c
> +++ b/sys/dev/xen/blkfront/blkfront.c
> @@ -68,6 +68,8 @@ __FBSDID("$FreeBSD$");
>  
>  #include "xenbus_if.h"
>  
> +static int blkfront_suspend_cancelled = 0;

I see that you add one of those to each frontend you add cancel suspend
support, together with a *_set_suspend_cancel function. Wouldn't it be easier
to declare a global suspend_cancelled variable in xen/control.c and export that
to all the frontends that need it? It should also be a bool type instead of
int.

You can clear that variable after DEVICE_RESUME has finished.

>  /*--- Forward Declarations 
> ---*/
>  static void xbd_closing(device_t);
>  static void xbd_startio(struct xbd_softc *sc);
> @@ -1417,10 +1419,21 @@ xbd_suspend(device_t dev)
>   return (retval);
>  }
>  
> +void xbd_set_suspend_cancel(void)
> +{
> + blkfront_suspend_cancelled = 1;
> +}
> +
>  static int
>  xbd_resume(device_t dev)
>  {
>   struct xbd_softc *sc = device_get_softc(dev);
> + 

Stray tab.

> + if(blkfront_suspend_cancelled == 1) {
  ^ missing space between "if" and "(".

> + sc->xbd_state = XBD_STATE_CONNECTED;
> + blkfront_suspend_cancelled = 0;
> + return (0);
> + }
>  
>   DPRINTK("xbd_resume: %s\n", xenbus_get_node(dev));
>  
> diff --git a/sys/dev/xen/control/control.c b/sys/dev/xen/control/control.c
> index bc0609d..b500100 100644
> --- a/sys/dev/xen/control/control.c
> +++ b/sys/dev/xen/control/control.c
> @@ -400,7 +400,9 @@ xctrl_suspend()
>   /*
>* Reset grant table info.
>*/
> - gnttab_resume();
> + if(suspend_cancelled == 0) {
  ^ missing space.

> + gnttab_resume();
> + }
>  
>  #ifdef SMP
>   if (smp_started && !CPU_EMPTY(_suspend_map)) {
> @@ -416,6 +418,11 @@ xctrl_suspend()
>* FreeBSD really needs to add DEVICE_SUSPEND_CANCEL or
>* similar.
>*/
> + if(suspend_cancelled == 1) {
> + xenbusb_set_suspend_cancel();
> + xbd_set_suspend_cancel();
> + xn_set_suspend_cancel();
> + }

If you make suspend_cancelled global you wouldn't need to call all of those
functions.

>   mtx_lock();
>   DEVICE_RESUME(root_bus);
>   mtx_unlock();
> diff --git a/sys/dev/xen/netfront/netfront.c b/sys/dev/xen/netfront/netfront.c
> index d89c0e0..0caaa2c 100644
> --- a/sys/dev/xen/netfront/netfront.c
> +++ b/sys/dev/xen/netfront/netfront.c
> @@ -98,6 +98,8 @@ __FBSDID("$FreeBSD$");
>  #define NET_TX_RING_SIZE __RING_SIZE((netif_tx_sring_t *)0, PAGE_SIZE)
>  #define NET_RX_RING_SIZE __RING_SIZE((netif_rx_sring_t *)0, PAGE_SIZE)
>  
> +static int netfront_suspend_cancelled = 0;
> +
>  #if __FreeBSD_version >= 70
>  /*
>   * Should the driver do LRO on the RX end
> @@ -192,6 +194,10 @@ static int xennet_get_responses(struct netfront_info *np,
>   struct netfront_rx_info *rinfo, RING_IDX rp, RING_IDX *cons,
>   struct mbuf 

Re: [PATCH]netfront: need release all resources after adding and removing NICs time and again

[Liuyingdong]

Hello Roger,
Thank you for the time and patience you devoted to reading my messages 
and e-mails. I really appreciate that.
I can't use git send-email so I attach the patches directly. In the 
0001 patch I introduce suspend_cancel mechanism for frontend devices and in the 
0002 patch I release all resources after hot unplug NICs.

Note: These two patches is on the base of the origin/release/10.2.0 branch 
and the 0002 patch is made after the 0001 patch.

-邮件原件-
发件人: freebsd xen [mailto:roger@citrix.com] 
发送时间: 2016年12月13日 22:29
收件人: Liuyingdong
抄送: freebsd-xen@freebsd.org; Zhaojun (Euler); Suoben; Ouyangzhaowei (Charles)
主题: Re: [PATCH]netfront: need release all resources after adding and removing 
NICs time and again

On Tue, Dec 13, 2016 at 02:03:08PM +0800, liuyingdong wrote:
> Hello Roger,
> I want to know how about this patch,Please let me know if you have any 
> questions.
> Thanks.

Hello,

Thanks for the patches! This one is looking fine, it's just that I'm a little 
bit busy at the moment, and there are some issues in HEAD related to Xen that I 
would like to fix before pushing anything new.

In any case, I see that you are sending the patches using Thunderbird, which is 
not ideal (MUAs tend to mangle patches). The preferred way for sending patches 
is using "git send-email"[0] directly. There are also several tutorials online 
that will help you setup git send-email correctly. If there's some reason why 
you can't use git send-email I would recommend that you also attach the patches 
directly to emails, that way they won't probably get mangled.

Roger.

[0] https://git-scm.com/docs/git-send-email


0001-introduce-suspend-cancel-mechanism-for-frontend-devices.patch
Description: 0001-introduce-suspend-cancel-mechanism-for-frontend-devices.patch


0002-netfront-need-release-all-resources-after-hot-plug.patch
Description: 0002-netfront-need-release-all-resources-after-hot-plug.patch
___
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: [PATCH]netfront: need release all resources after adding and removing NICs time and again

[freebsd xen]

On Tue, Dec 13, 2016 at 02:03:08PM +0800, liuyingdong wrote:
> Hello Roger,
> I want to know how about this patch,Please let me know if you have any 
> questions.
> Thanks.

Hello,

Thanks for the patches! This one is looking fine, it's just that I'm a little 
bit busy at the moment, and there are some issues in HEAD related to Xen that I 
would like to fix before pushing anything new.

In any case, I see that you are sending the patches using Thunderbird, which is 
not ideal (MUAs tend to mangle patches). The preferred way for sending patches 
is using "git send-email"[0] directly. There are also several tutorials online 
that will help you setup git send-email correctly. If there's some reason why 
you can't use git send-email I would recommend that you also attach the patches 
directly to emails, that way they won't probably get mangled.

Roger.

[0] https://git-scm.com/docs/git-send-email
___
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: [PATCH]netfront: need release all resources after adding and removing NICs time and again

[liuyingdong]

Hello Roger,
I want to know how about this patch,Please let me know if you have any 
questions.
Thanks.

  Yours

On 2016/11/11 10:04, liuyingdong wrote:
> Hello Roger,
>   Sorry, I am a freshman. I ran the following command to git kernel 
> source of freebsd 10.2.0:
>   git clone https://github.com/freebsd/freebsd.git
>   git checkout origin/release/10.2.0
>   On the base of the above, I make a patch file as fellows:
> 
> diff --git a/sys/dev/xen/netfront/netfront.c b/sys/dev/xen/netfront/netfront.c
> index d89c0e0..903eb98 100644
> --- a/sys/dev/xen/netfront/netfront.c
> +++ b/sys/dev/xen/netfront/netfront.c
> @@ -2172,6 +2172,46 @@ netfront_detach(device_t dev)
>  }
> 
>  static void
> +netif_release_rx_bufs_copy(struct netfront_info *np)
> +{
> + struct mbuf *m;
> + int i, ref;
> + int busy = 0, inuse = 0;
> +
> + XN_RX_LOCK(np);
> +
> + for (i = 0; i < NET_RX_RING_SIZE; i++) {
> + ref = np->grant_rx_ref[i];
> +
> + if (ref == GRANT_REF_INVALID)
> + continue;
> +
> + inuse++;
> +
> + m = np->rx_mbufs[i];
> +
> + if (!gnttab_end_foreign_access_ref(ref))
> + {
> + busy++;
> + continue;
> + }
> +
> + gnttab_release_grant_reference(>gref_rx_head, ref);
> + np->grant_rx_ref[i] = GRANT_REF_INVALID;
> + add_id_to_freelist(np->rx_mbufs, i);
> +
> + m_freem(m);
> + }
> +
> + if (busy)
> + device_printf(np->xbdev, "Unable to release %d of %d "
> + "inuse grant references out of %ld total.\n",
> + busy, inuse, NET_RX_RING_SIZE);
> + 
> + XN_RX_UNLOCK(np);
> +}
> +
> +static void
>  netif_free(struct netfront_info *info)
>  {
>   XN_LOCK(info);
> @@ -2185,6 +2225,13 @@ netif_free(struct netfront_info *info)
>   info->xn_ifp = NULL;
>   }
>   ifmedia_removeall(>sc_media);
> + 
> + netif_release_tx_bufs(info);
> + if (info->copying_receiver)
> + netif_release_rx_bufs_copy(info);
> +
> + gnttab_free_grant_references(info->gref_tx_head);
> + gnttab_free_grant_references(info->gref_rx_head);
>  }
> 
>  static void
> 
> 
> Yours Yingdong Liu
> 
> 
> -邮件原件-
> 发件人: roger@citrix.com [mailto:roger@citrix.com]
> 发送时间: 2016年11月8日 21:05
> 收件人: Liuyingdong
> 抄送: freebsd-xen@freebsd.org; Zhaojun (Euler); Suoben
> 主题: Re: [PATCH]netfront: need release all resources after adding and removing 
> NICs time and again
> 
> On Fri, Nov 04, 2016 at 07:43:41AM +, Liuyingdong wrote:
>> On xen,freebsd 10.2 virtual machines hang after adding and removing NICs 
>> time and again(more than 30 times).
>> I found error log is as follows:
>> "netfront can't alloc tx grant refs"
> 
> Hello,
> 
> Thanks for the patch, although I'm not able to import it, git complain with:
> 
> patch:  malformed patch at line 7: }
> 
> Could you please resend it using git send-email?
> 
> I also have a couple of comments below regarding style.
> 
>>
>> Signed-off-by: Yingdong Liu<liuyingd...@huawei.com>
>>
>> diff --git a/dev/xen/netfront/netfront.c b/dev/xen/netfront/netfront.c
>> index 5497139..e96bbba 100644
>> --- a/dev/xen/netfront/netfront.c
>> +++ b/dev/xen/netfront/netfront.c
>> @@ -790,6 +790,44 @@ netif_release_tx_bufs(struct netfront_info *np)
>>}
>> }
> 
> Missing new line.
> 
>> +static void
> netif_release_rx_bufs_copy(struct netfront_info *np)
> 
> This needs to be on a new line.
> 
>> +{
>> +   struct mbuf *m;
>> +   int i, ref;
>> +   int busy = 0, inuse = 0;
>> +
>> +   XN_RX_LOCK(np);
>> +
>> +   for (i = 0; i < NET_RX_RING_SIZE; i++) {
>> +ref = np->grant_rx_ref[i];
>> +
>> +if (ref == GRANT_REF_INVALID)
>> +  continue;
>> +
>> +inuse++;
>> +
>> +m = np->rx_mbufs[i];
>> +
>> +if (!gnttab_end_foreign_access_ref(ref))
>> +{
>> +  busy++;
>> +  continue;
>> +}
>> +
>> +gnttab_release_grant_reference(>gref_rx_head, ref);
>> +

Re: [PATCH]netfront: need release all resources after adding and removing NICs time and again

[liuyingdong]

Hello Roger,
Sorry, I am a freshman. I ran the following command to git kernel 
source of freebsd 10.2.0:
git clone https://github.com/freebsd/freebsd.git
git checkout origin/release/10.2.0
On the base of the above, I make a patch file as fellows:

diff --git a/sys/dev/xen/netfront/netfront.c b/sys/dev/xen/netfront/netfront.c
index d89c0e0..903eb98 100644
--- a/sys/dev/xen/netfront/netfront.c
+++ b/sys/dev/xen/netfront/netfront.c
@@ -2172,6 +2172,46 @@ netfront_detach(device_t dev)
 }

 static void
+netif_release_rx_bufs_copy(struct netfront_info *np)
+{
+   struct mbuf *m;
+   int i, ref;
+   int busy = 0, inuse = 0;
+
+   XN_RX_LOCK(np);
+
+   for (i = 0; i < NET_RX_RING_SIZE; i++) {
+   ref = np->grant_rx_ref[i];
+
+   if (ref == GRANT_REF_INVALID)
+   continue;
+
+   inuse++;
+
+   m = np->rx_mbufs[i];
+
+   if (!gnttab_end_foreign_access_ref(ref))
+   {
+   busy++;
+   continue;
+   }
+
+   gnttab_release_grant_reference(>gref_rx_head, ref);
+   np->grant_rx_ref[i] = GRANT_REF_INVALID;
+   add_id_to_freelist(np->rx_mbufs, i);
+
+   m_freem(m);
+   }
+
+   if (busy)
+   device_printf(np->xbdev, "Unable to release %d of %d "
+   "inuse grant references out of %ld total.\n",
+   busy, inuse, NET_RX_RING_SIZE);
+   
+   XN_RX_UNLOCK(np);
+}
+
+static void
 netif_free(struct netfront_info *info)
 {
XN_LOCK(info);
@@ -2185,6 +2225,13 @@ netif_free(struct netfront_info *info)
info->xn_ifp = NULL;
}
ifmedia_removeall(>sc_media);
+   
+   netif_release_tx_bufs(info);
+   if (info->copying_receiver)
+   netif_release_rx_bufs_copy(info);
+
+   gnttab_free_grant_references(info->gref_tx_head);
+   gnttab_free_grant_references(info->gref_rx_head);
 }

 static void


Yours Yingdong Liu


-邮件原件-
发件人: roger@citrix.com [mailto:roger@citrix.com]
发送时间: 2016年11月8日 21:05
收件人: Liuyingdong
抄送: freebsd-xen@freebsd.org; Zhaojun (Euler); Suoben
主题: Re: [PATCH]netfront: need release all resources after adding and removing 
NICs time and again

On Fri, Nov 04, 2016 at 07:43:41AM +, Liuyingdong wrote:
> On xen,freebsd 10.2 virtual machines hang after adding and removing NICs time 
> and again(more than 30 times).
> I found error log is as follows:
> "netfront can't alloc tx grant refs"

Hello,

Thanks for the patch, although I'm not able to import it, git complain with:

patch:  malformed patch at line 7: }

Could you please resend it using git send-email?

I also have a couple of comments below regarding style.

>
> Signed-off-by: Yingdong Liu<liuyingd...@huawei.com>
>
> diff --git a/dev/xen/netfront/netfront.c b/dev/xen/netfront/netfront.c
> index 5497139..e96bbba 100644
> --- a/dev/xen/netfront/netfront.c
> +++ b/dev/xen/netfront/netfront.c
> @@ -790,6 +790,44 @@ netif_release_tx_bufs(struct netfront_info *np)
>}
> }

Missing new line.

> +static void
netif_release_rx_bufs_copy(struct netfront_info *np)

This needs to be on a new line.

> +{
> +   struct mbuf *m;
> +   int i, ref;
> +   int busy = 0, inuse = 0;
> +
> +   XN_RX_LOCK(np);
> +
> +   for (i = 0; i < NET_RX_RING_SIZE; i++) {
> +ref = np->grant_rx_ref[i];
> +
> +if (ref == GRANT_REF_INVALID)
> +  continue;
> +
> +inuse++;
> +
> +m = np->rx_mbufs[i];
> +
> +if (!gnttab_end_foreign_access_ref(ref))
> +{
> +  busy++;
> +  continue;
> +}
> +
> +gnttab_release_grant_reference(>gref_rx_head, ref);
> +np->grant_rx_ref[i] = GRANT_REF_INVALID;
> +add_id_to_freelist(np->rx_mbufs, i);
> +
> +m_freem(m);
> +   }
> +
> +   if (busy)
> +DPRINTK("%s: Unable to release %d of %d inuse grant 
> references out of %ld total.\n",
> +  __FUNCTION__, busy, inuse,
> + NET_RX_RING_SIZE);

Please use device_printf for this instead of the function name. Also, could you 
align the string so it doesn't extend past 80 characters?

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"