Re: [PATCH] xen-netfront: fix xennet_start_xmit()'s return type

[Juergen Gross]

On 24/04/18 15:18, Luc Van Oostenryck wrote:
> The method ndo_start_xmit() is defined as returning an 'netdev_tx_t',
> which is a typedef for an enum type, but the implementation in this
> driver returns an 'int'.
> 
> Fix this by returning 'netdev_tx_t' in this driver too.
> 
> Signed-off-by: Luc Van Oostenryck 

Pushed to xen/tip.git for-linus-4.18


Juergen

Re: [PATCH 2/3] xen netback: add fault injection facility

[Juergen Gross]

On 20/04/18 14:52, stask...@amazon.com wrote:
> On 04/20/18 13:25, Juergen Gross wrote:
>> On 20/04/18 12:47, Stanislav Kinsburskii wrote:
>>> +    for (fi = 0; fi < XENVIF_FI_MAX; fi++) {
>>> +    vfi->faults[fi] = xen_fi_dir_add(vfi->dir,
>>> +    xenvif_fi_names[fi]);
>> How does this work? xenvif_fi_names[] is an empty array and this is the
>> only reference to it. Who is allocating the memory for that array?
> 
> Well, it works in the way one adds a var to enum (which is used as a key
> later) and a corresponding string into the array (which is used as a
> name for the fault directory in sysfs).

Then you should size the array via XENVIF_FI_MAX.


Juergen

Re: [PATCH 3/3] xen blkback: add fault injection facility

[Juergen Gross]

On 20/04/18 12:47, Stanislav Kinsburskii wrote:
> This patch adds wrapper helpers around generic Xen fault inject
> facility.
> The major reason is to keep all the module fault injection directories
> in a dedicated subdirectory instead of Xen fault inject root.
> 
> IOW, when using these helpers, per-device and named by device name
> fault injection control directories will appear under the following
> directory:
> - /sys/kernel/debug/xen/fault_inject/xen-blkback/
> instead of:
> - /sys/kernel/debug/xen/fault_inject/
> 
> Signed-off-by: Stanislav Kinsburskii 
> CC: Jens Axboe 
> CC: Konrad Rzeszutek Wilk 
> CC: "Roger Pau Monné" 
> CC: linux-ker...@vger.kernel.org
> CC: linux-bl...@vger.kernel.org
> CC: xen-de...@lists.xenproject.org
> CC: Stanislav Kinsburskii 
> CC: David Woodhouse 

This is an exact copy of the netback patch apart from the names.

I don't like adding multiple copies of the same coding to the tree.


Juergen

Re: [PATCH 2/3] xen netback: add fault injection facility

[Juergen Gross]

On 20/04/18 12:47, Stanislav Kinsburskii wrote:
> This patch adds wrapper helpers around generic Xen fault inject facility.
> The major reason is to keep all the module fault injection directories
> in a dedicated subdirectory instead of Xen fault inject root.
> 
> IOW, when using these helpers, per-device and named by device name fault
> injection control directories will appear under the following directory:
> - /sys/kernel/debug/xen/fault_inject/xen-netback/
> instead of:
> - /sys/kernel/debug/xen/fault_inject/
> 
> Signed-off-by: Stanislav Kinsburskii <stask...@amazon.com>
> CC: Wei Liu <wei.l...@citrix.com>
> CC: Paul Durrant <paul.durr...@citrix.com>
> CC: "David S. Miller" <da...@davemloft.net>
> CC: Matteo Croce <mcr...@redhat.com>
> CC: Stefan Hajnoczi <stefa...@redhat.com>
> CC: Daniel Borkmann <dan...@iogearbox.net>
> CC: Gerard Garcia <ggar...@deic.uab.cat>
> CC: David Ahern <d...@cumulusnetworks.com>
> CC: Juergen Gross <jgr...@suse.com>
> CC: Amir Levy <amir.jer.l...@intel.com>
> CC: Jakub Kicinski <jakub.kicin...@netronome.com>
> CC: linux-ker...@vger.kernel.org
> CC: netdev@vger.kernel.org
> CC: xen-de...@lists.xenproject.org
> CC: Stanislav Kinsburskii <stask...@amazon.com>
> CC: David Woodhouse <d...@amazon.co.uk>
> ---
>  drivers/net/Kconfig  |8 ++
>  drivers/net/xen-netback/Makefile |1 
>  drivers/net/xen-netback/common.h |3 +
>  drivers/net/xen-netback/netback.c|3 +
>  drivers/net/xen-netback/netback_fi.c |  119 
> ++
>  drivers/net/xen-netback/netback_fi.h |   35 ++
>  drivers/net/xen-netback/xenbus.c |6 ++
>  7 files changed, 175 insertions(+)
>  create mode 100644 drivers/net/xen-netback/netback_fi.c
>  create mode 100644 drivers/net/xen-netback/netback_fi.h
> 
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 8918466..5cc9acd 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -465,6 +465,14 @@ config XEN_NETDEV_BACKEND
> compile this driver as a module, chose M here: the module
> will be called xen-netback.
>  
> +config XEN_NETDEV_BACKEND_FAULT_INJECTION
> +   bool "Xen net-device backend driver fault injection"
> +   depends on XEN_NETDEV_BACKEND
> +   depends on XEN_FAULT_INJECTION
> +   default n
> +   help
> + Allow to inject errors to Xen backend network driver
> +
>  config VMXNET3
>   tristate "VMware VMXNET3 ethernet driver"
>   depends on PCI && INET
> diff --git a/drivers/net/xen-netback/Makefile 
> b/drivers/net/xen-netback/Makefile
> index d49798a..28abcdc 100644
> --- a/drivers/net/xen-netback/Makefile
> +++ b/drivers/net/xen-netback/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_XEN_NETDEV_BACKEND) := xen-netback.o
>  
>  xen-netback-y := netback.o xenbus.o interface.o hash.o rx.o
> +xen-netback-$(CONFIG_XEN_NETDEV_BACKEND_FAULT_INJECTION) += netback_fi.o
> diff --git a/drivers/net/xen-netback/common.h 
> b/drivers/net/xen-netback/common.h
> index a46a1e9..30d676d 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -286,6 +286,9 @@ struct xenvif {
>  
>  #ifdef CONFIG_DEBUG_FS
>   struct dentry *xenvif_dbg_root;
> +#ifdef CONFIG_XEN_NETDEV_BACKEND_FAULT_INJECTION
> + void *fi_info;
> +#endif
>  #endif
>  
>   struct xen_netif_ctrl_back_ring ctrl;
> diff --git a/drivers/net/xen-netback/netback.c 
> b/drivers/net/xen-netback/netback.c
> index a27daa2..ecc416e 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -33,6 +33,7 @@
>   */
>  
>  #include "common.h"
> +#include "netback_fi.h"
>  
>  #include 
>  #include 
> @@ -1649,6 +1650,7 @@ static int __init netback_init(void)
>   PTR_ERR(xen_netback_dbg_root));
>  #endif /* CONFIG_DEBUG_FS */
>  
> + (void) xen_netbk_fi_init();

This is the only usage of xen_netbk_fi_init(). Why don't you make it
return void from the beginning?

>   return 0;
>  
>  failed_init:
> @@ -1659,6 +1661,7 @@ module_init(netback_init);
>  
>  static void __exit netback_fini(void)
>  {
> + xen_netbk_fi_fini();
>  #ifdef CONFIG_DEBUG_FS
>   if (!IS_ERR_OR_NULL(xen_netback_dbg_root))
>   debugfs_remove_recursive(xen_netback_dbg_root);
> diff --git a/drivers/net/xen-netback/netback_fi.c 
> b/drivers/net/xen-netback/netback_fi.c
> new file mode 100644
> index 000..47541d0
> --- /dev/null
> +++ b/drivers/net/xen-netback/netback_

Re: [PATCH 1/3] xen: add generic fault injection facility

[Juergen Gross]

On 20/04/18 12:47, Stanislav Kinsburskii wrote:
> The overall idea of this patch is to add a generic fault injection facility
> to Xen, which later can be used in various places by different Xen parts.
> 
> Core implementation ideas:
> 
> - The facility build is controlled by boolean config
>   CONFIG_XEN_FAULT_INJECTION option ("N" by default).
> 
> - All fault injection logic is located in an optionally compiled separated
>   file.
> 
> - Fault injection attribute and control directory creation and destruction
>   are wrapped with helpers, producing and accepting a pointer to an opaque
>   object thus making all the rest of code independent on fault injection
>   engine.
> 
> When enabled Xen root fault injection directory appears:
> 
> - /sys/kernel/debug/xen/fault_inject/
> 
> The falicity provides the following helpers (exported to be accessible in
> modules):
> 
> - xen_fi_add(name) - adds fault injection control directory "name" to Xen
>   root fault injection directory
> 
> - xen_fi_dir_create(name) - allows to create a subdirectory "name" in Xen
>   root fault injection directory.
> 
> - xen_fi_dir_add(dir, name) - adds fault injection control directory "name"
>   to directory "dir"
> 
> - xen_should_fail(fi) - check whether fi hav to fail.
> 
> Signed-off-by: Stanislav Kinsburskii <stask...@amazon.com>
> CC: Boris Ostrovsky <boris.ostrov...@oracle.com>
> CC: Juergen Gross <jgr...@suse.com>
> CC: Thomas Gleixner <t...@linutronix.de>
> CC: Ingo Molnar <mi...@redhat.com>
> CC: "H. Peter Anvin" <h...@zytor.com>
> CC: x...@kernel.org
> CC: xen-de...@lists.xenproject.org
> CC: linux-ker...@vger.kernel.org
> CC: Stanislav Kinsburskii <stask...@amazon.com>
> CC: David Woodhouse <d...@amazon.co.uk>
> ---
>  arch/x86/xen/Kconfig|7 +++
>  arch/x86/xen/Makefile   |1 
>  arch/x86/xen/fault_inject.c |  109 
> +++
>  include/xen/fault_inject.h  |   45 ++
>  4 files changed, 162 insertions(+)
>  create mode 100644 arch/x86/xen/fault_inject.c
>  create mode 100644 include/xen/fault_inject.h
> 
> diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
> index c1f98f3..483fc16 100644
> --- a/arch/x86/xen/Kconfig
> +++ b/arch/x86/xen/Kconfig
> @@ -77,3 +77,10 @@ config XEN_PVH
>   bool "Support for running as a PVH guest"
>   depends on XEN && XEN_PVHVM && ACPI
>   def_bool n
> +
> +config XEN_FAULT_INJECTION
> + bool "Enable Xen fault injection"
> + depends on FAULT_INJECTION_DEBUG_FS
> + default n
> + help
> +   Enable Xen fault injection facility

Why for x86 only? I'd rather add this under drivers/xen

> diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
> index d83cb54..3158fe1 100644
> --- a/arch/x86/xen/Makefile
> +++ b/arch/x86/xen/Makefile
> @@ -34,3 +34,4 @@ obj-$(CONFIG_XEN_DOM0)  += vga.o
>  obj-$(CONFIG_SWIOTLB_XEN)+= pci-swiotlb-xen.o
>  obj-$(CONFIG_XEN_EFI)+= efi.o
>  obj-$(CONFIG_XEN_PVH)+= xen-pvh.o
> +obj-$(CONFIG_XEN_FAULT_INJECTION)+= fault_inject.o
> diff --git a/arch/x86/xen/fault_inject.c b/arch/x86/xen/fault_inject.c
> new file mode 100644
> index 000..ecf0f7c
> --- /dev/null
> +++ b/arch/x86/xen/fault_inject.c
> @@ -0,0 +1,109 @@
> +/*
> + * Fauit injection interface for Xen virtual block devices
> + *
> + * Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation; or, when distributed
> + * separately from the Linux kernel or incorporated into other
> + * software packages, subject to the following license:

Please use the appropriate SPDX header instead of the full GPL2
boilerplate.


Juergen

Re: [PATCH] xen-netfront: Fix hang on device removal

[Juergen Gross]

On 28/02/18 13:23, Jason Andryuk wrote:
> A toolstack may delete the vif frontend and backend xenstore entries
> while xen-netfront is in the removal code path.  In that case, the
> checks for xenbus_read_driver_state would return XenbusStateUnknown, and
> xennet_remove would hang indefinitely.  This hang prevents system
> shutdown.
> 
> xennet_remove must be able to handle XenbusStateUnknown, and
> netback_changed must also wake up the wake_queue for that state as well.
> 
> Fixes: 5b5971df3bc2 ("xen-netfront: remove warning when unloading module")
> 
> Signed-off-by: Jason Andryuk 
> Cc: Eduardo Otubo 

Committed to xen/tip for-linus-4.16a


Juergen

Re: [PATCHv2] xen-netfront: remove warning when unloading module

[Juergen Gross]

On 23/11/17 15:18, Eduardo Otubo wrote:
> v2:
>  * Replace busy wait with wait_event()/wake_up_all()
>  * Cannot garantee that at the time xennet_remove is called, the
>xen_netback state will not be XenbusStateClosed, so added a
>condition for that
>  * There's a small chance for the xen_netback state is
>XenbusStateUnknown by the time the xen_netfront switches to Closed,
>so added a condition for that.
> 
> When unloading module xen_netfront from guest, dmesg would output
> warning messages like below:
> 
>   [  105.236836] xen:grant_table: WARNING: g.e. 0x903 still in use!
>   [  105.236839] deferring g.e. 0x903 (pfn 0x35805)
> 
> This problem relies on netfront and netback being out of sync. By the time
> netfront revokes the g.e.'s netback didn't have enough time to free all of
> them, hence displaying the warnings on dmesg.
> 
> The trick here is to make netfront to wait until netback frees all the g.e.'s
> and only then continue to cleanup for the module removal, and this is done by
> manipulating both device states.
> 
> Signed-off-by: Eduardo Otubo <ot...@redhat.com>

Acked-by: Juergen Gross <jgr...@suse.com>


Juergen

Re: [PATCH] xen-netfront: remove warning when unloading module

[Juergen Gross]

On 20/11/17 11:49, Wei Liu wrote:
> CC netfront maintainers.
> 
> On Mon, Nov 20, 2017 at 11:41:09AM +0100, Eduardo Otubo wrote:
>> When unloading module xen_netfront from guest, dmesg would output
>> warning messages like below:
>>
>>   [  105.236836] xen:grant_table: WARNING: g.e. 0x903 still in use!
>>   [  105.236839] deferring g.e. 0x903 (pfn 0x35805)
>>
>> This problem relies on netfront and netback being out of sync. By the time
>> netfront revokes the g.e.'s netback didn't have enough time to free all of
>> them, hence displaying the warnings on dmesg.
>>
>> The trick here is to make netfront to wait until netback frees all the g.e.'s
>> and only then continue to cleanup for the module removal, and this is done by
>> manipulating both device states.
>>
>> Signed-off-by: Eduardo Otubo 
>> ---
>>  drivers/net/xen-netfront.c | 11 +++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
>> index 8b8689c6d887..b948e2a1ce40 100644
>> --- a/drivers/net/xen-netfront.c
>> +++ b/drivers/net/xen-netfront.c
>> @@ -2130,6 +2130,17 @@ static int xennet_remove(struct xenbus_device *dev)
>>  
>>  dev_dbg(>dev, "%s\n", dev->nodename);
>>  
>> +xenbus_switch_state(dev, XenbusStateClosing);
>> +while (xenbus_read_driver_state(dev->otherend) != XenbusStateClosing){
>> +cpu_relax();
>> +schedule();
>> +}
>> +xenbus_switch_state(dev, XenbusStateClosed);
>> +while (dev->xenbus_state != XenbusStateClosed){
>> +cpu_relax();
>> +schedule();
>> +}

I really don't like the busy waits.

Can't you use e.g. a wait queue and wait_event_interruptible() instead?

BTW: what happens if the device is already in closed state if you enter
xennet_remove()? In case this is impossible, please add a comment to
indicate you've thought about that case.

Other than that: you should run ./scripts/checkpatch.p1 against your
patch to avoid common style problems.


Juergen

[PATCH v2] i40e/i40evf: use cpumask_copy() for assigning cpumask

[Juergen Gross]

Using direct assignment for a cpumask is wrong, cpumask_copy() should
be used instead.

Otherwise crashes like the following might happen:

[62792.326374] BUG: unable to handle kernel paging request at 8800049ff000
[62792.340118] IP: [] i40e_irq_affinity_notify+0x11/0x20 
[i40e]
...
[62792.810770] Call Trace:
[62792.815722]  [] irq_affinity_notify+0xb5/0xf0
[62792.827593]  [] process_one_work+0x14e/0x410
[62792.839282]  [] worker_thread+0x116/0x490
[62792.850459]  [] kthread+0xc7/0xe0
[62792.860255]  [] ret_from_fork+0x3f/0x70
[62792.871996] DWARF2 unwinder stuck at ret_from_fork+0x3f/0x70

Fixes: 96db776a3682 ("i40e/i40evf: fix interrupt affinity bug")
Cc: <sta...@vger.kernel.org> # 4.10+
Signed-off-by: Juergen Gross <jgr...@suse.com>
---
V2: enhance commit message, merge patches
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c 
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 2db93d3f6d23..c0e42d162c7c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3495,7 +3495,7 @@ static void i40e_irq_affinity_notify(struct 
irq_affinity_notify *notify,
struct i40e_q_vector *q_vector =
container_of(notify, struct i40e_q_vector, affinity_notify);
 
-   q_vector->affinity_mask = *mask;
+   cpumask_copy(_vector->affinity_mask, mask);
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c 
b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
index 7c213a347909..a4b60367ecce 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
@@ -520,7 +520,7 @@ static void i40evf_irq_affinity_notify(struct 
irq_affinity_notify *notify,
struct i40e_q_vector *q_vector =
container_of(notify, struct i40e_q_vector, affinity_notify);
 
-   q_vector->affinity_mask = *mask;
+   cpumask_copy(_vector->affinity_mask, mask);
 }
 
 /**
-- 
2.12.3

[PATCH] net/i40evf: use cpumask_copy() for assigning cpumask

[Juergen Gross]

Using direct assignment for a cpumask is wrong, cpumask_copy() should
be used instead.

Cc: sta...@vger.kernel.org
Signed-off-by: Juergen Gross <jgr...@suse.com>
---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c 
b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
index 7c213a347909..a4b60367ecce 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
@@ -520,7 +520,7 @@ static void i40evf_irq_affinity_notify(struct 
irq_affinity_notify *notify,
struct i40e_q_vector *q_vector =
container_of(notify, struct i40e_q_vector, affinity_notify);
 
-   q_vector->affinity_mask = *mask;
+   cpumask_copy(_vector->affinity_mask, mask);
 }
 
 /**
-- 
2.12.3

[PATCH] net/i40e: use cpumask_copy() for assigning cpumask

[Juergen Gross]

Using direct assignment for a cpumask is wrong, cpumask_copy() should
be used instead.

Cc: sta...@vger.kernel.org
Signed-off-by: Juergen Gross <jgr...@suse.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c 
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 2db93d3f6d23..c0e42d162c7c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3495,7 +3495,7 @@ static void i40e_irq_affinity_notify(struct 
irq_affinity_notify *notify,
struct i40e_q_vector *q_vector =
container_of(notify, struct i40e_q_vector, affinity_notify);
 
-   q_vector->affinity_mask = *mask;
+   cpumask_copy(_vector->affinity_mask, mask);
 }
 
 /**
-- 
2.12.3

Re: [PATCH] xen/9pfs: select CONFIG_XEN_XENBUS_FRONTEND

[Juergen Gross]

On 20/04/17 01:10, Stefano Stabellini wrote:
> Juergen, I have committed this patch to for-linus-4.12 and linux-next, I
> hope that's OK.

Sure.


Juergen

> 
> Og Wed, 19 Apr 2017, Stefano Stabellini wrote:
>> On Wed, 19 Apr 2017, Arnd Bergmann wrote:
>>> All Xen frontends need to select this symbol to avoid a link error:
>>>
>>> net/built-in.o: In function `p9_trans_xen_init':
>>> :(.text+0x149e9c): undefined reference to `__xenbus_register_frontend'
>>>
>>> Fixes: d4b40a02f837 ("xen/9pfs: build 9pfs Xen transport driver")
>>> Signed-off-by: Arnd Bergmann 
>>
>> Thank you for the fix!
>>
>> Reviewed-by: Stefano Stabellini 
>>> ---
>>>  net/9p/Kconfig | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/net/9p/Kconfig b/net/9p/Kconfig
>>> index 3f286f1bd1d3..e6014e0e51f7 100644
>>> --- a/net/9p/Kconfig
>>> +++ b/net/9p/Kconfig
>>> @@ -24,6 +24,7 @@ config NET_9P_VIRTIO
>>>  
>>>  config NET_9P_XEN
>>> depends on XEN
>>> +   select XEN_XENBUS_FRONTEND
>>> tristate "9P Xen Transport"
>>> help
>>>   This builds support for a transport for 9pfs between
>>> -- 
>>> 2.9.0
>>>
>>
> 

BUG due to "xen-netback: protect resource cleaning on XenBus disconnect"

[Juergen Gross]

With commits f16f1df65 and 9a6cdf52b we get in our Xen testing:

[  174.512861] switch: port 2(vif3.0) entered disabled state
[  174.522735] BUG: sleeping function called from invalid context at
/home/build/linux-linus/mm/vmalloc.c:1441
[  174.523451] in_atomic(): 1, irqs_disabled(): 0, pid: 28, name: xenwatch
[  174.524131] CPU: 1 PID: 28 Comm: xenwatch Tainted: GW
4.10.0upstream-11073-g4977ab6-dirty #1
[  174.524819] Hardware name: MSI MS-7680/H61M-P23 (MS-7680), BIOS V17.0
03/14/2011
[  174.525517] Call Trace:
[  174.526217]  show_stack+0x23/0x60
[  174.526899]  dump_stack+0x5b/0x88
[  174.527562]  ___might_sleep+0xde/0x130
[  174.528208]  __might_sleep+0x35/0xa0
[  174.528840]  ? _raw_spin_unlock_irqrestore+0x13/0x20
[  174.529463]  ? __wake_up+0x40/0x50
[  174.530089]  remove_vm_area+0x20/0x90
[  174.530724]  __vunmap+0x1d/0xc0
[  174.531346]  ? delete_object_full+0x13/0x20
[  174.531973]  vfree+0x40/0x80
[  174.532594]  set_backend_state+0x18a/0xa90
[  174.533221]  ? dwc_scan_descriptors+0x24d/0x430
[  174.533850]  ? kfree+0x5b/0xc0
[  174.534476]  ? xenbus_read+0x3d/0x50
[  174.535101]  ? xenbus_read+0x3d/0x50
[  174.535718]  ? xenbus_gather+0x31/0x90
[  174.536332]  ? ___might_sleep+0xf6/0x130
[  174.536945]  frontend_changed+0x6b/0xd0
[  174.537565]  xenbus_otherend_changed+0x7d/0x80
[  174.538185]  frontend_changed+0x12/0x20
[  174.538803]  xenwatch_thread+0x74/0x110
[  174.539417]  ? woken_wake_function+0x20/0x20
[  174.540049]  kthread+0xe5/0x120
[  174.540663]  ? xenbus_printf+0x50/0x50
[  174.541278]  ? __kthread_init_worker+0x40/0x40
[  174.541898]  ret_from_fork+0x21/0x2c
[  174.548635] switch: port 2(vif3.0) entered disabled state

I believe calling vfree() when holding a spin_lock isn't a good idea.

Boris, this is the dumpdata failure:
FAILURE 4.10.0upstream-11073-g4977ab6-dirty(x86_64)
4.10.0upstream-11073-g4977ab6-dirty(i386)\: 2017-03-02 (tst007)


Juergen

Re: [Xen-devel] [PATCH net 2/2] xen-netback: don't vfree() queues under spinlock

[Juergen Gross]

On 02/03/17 13:54, Paul Durrant wrote:
> This leads to a BUG of the following form:
> 
> [  174.512861] switch: port 2(vif3.0) entered disabled state
> [  174.522735] BUG: sleeping function called from invalid context at
> /home/build/linux-linus/mm/vmalloc.c:1441
> [  174.523451] in_atomic(): 1, irqs_disabled(): 0, pid: 28, name: xenwatch
> [  174.524131] CPU: 1 PID: 28 Comm: xenwatch Tainted: GW
> 4.10.0upstream-11073-g4977ab6-dirty #1
> [  174.524819] Hardware name: MSI MS-7680/H61M-P23 (MS-7680), BIOS V17.0
> 03/14/2011
> [  174.525517] Call Trace:
> [  174.526217]  show_stack+0x23/0x60
> [  174.526899]  dump_stack+0x5b/0x88
> [  174.527562]  ___might_sleep+0xde/0x130
> [  174.528208]  __might_sleep+0x35/0xa0
> [  174.528840]  ? _raw_spin_unlock_irqrestore+0x13/0x20
> [  174.529463]  ? __wake_up+0x40/0x50
> [  174.530089]  remove_vm_area+0x20/0x90
> [  174.530724]  __vunmap+0x1d/0xc0
> [  174.531346]  ? delete_object_full+0x13/0x20
> [  174.531973]  vfree+0x40/0x80
> [  174.532594]  set_backend_state+0x18a/0xa90
> [  174.533221]  ? dwc_scan_descriptors+0x24d/0x430
> [  174.533850]  ? kfree+0x5b/0xc0
> [  174.534476]  ? xenbus_read+0x3d/0x50
> [  174.535101]  ? xenbus_read+0x3d/0x50
> [  174.535718]  ? xenbus_gather+0x31/0x90
> [  174.536332]  ? ___might_sleep+0xf6/0x130
> [  174.536945]  frontend_changed+0x6b/0xd0
> [  174.537565]  xenbus_otherend_changed+0x7d/0x80
> [  174.538185]  frontend_changed+0x12/0x20
> [  174.538803]  xenwatch_thread+0x74/0x110
> [  174.539417]  ? woken_wake_function+0x20/0x20
> [  174.540049]  kthread+0xe5/0x120
> [  174.540663]  ? xenbus_printf+0x50/0x50
> [  174.541278]  ? __kthread_init_worker+0x40/0x40
> [  174.541898]  ret_from_fork+0x21/0x2c
> [  174.548635] switch: port 2(vif3.0) entered disabled state
> 
> This patch defers the vfree() until after the spinlock is released.
> 
> Reported-by: Juergen Gross <jgr...@suse.com>
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>

Reviewed-by: Juergen Gross <jgr...@suse.com>


Juergen

Re: BUG due to "xen-netback: protect resource cleaning on XenBus disconnect"

[Juergen Gross]

On 02/03/17 13:06, Wei Liu wrote:
> On Thu, Mar 02, 2017 at 12:56:20PM +0100, Juergen Gross wrote:
>> With commits f16f1df65 and 9a6cdf52b we get in our Xen testing:
>>
>> [  174.512861] switch: port 2(vif3.0) entered disabled state
>> [  174.522735] BUG: sleeping function called from invalid context at
>> /home/build/linux-linus/mm/vmalloc.c:1441
>> [  174.523451] in_atomic(): 1, irqs_disabled(): 0, pid: 28, name: xenwatch
>> [  174.524131] CPU: 1 PID: 28 Comm: xenwatch Tainted: GW
>> 4.10.0upstream-11073-g4977ab6-dirty #1
>> [  174.524819] Hardware name: MSI MS-7680/H61M-P23 (MS-7680), BIOS V17.0
>> 03/14/2011
>> [  174.525517] Call Trace:
>> [  174.526217]  show_stack+0x23/0x60
>> [  174.526899]  dump_stack+0x5b/0x88
>> [  174.527562]  ___might_sleep+0xde/0x130
>> [  174.528208]  __might_sleep+0x35/0xa0
>> [  174.528840]  ? _raw_spin_unlock_irqrestore+0x13/0x20
>> [  174.529463]  ? __wake_up+0x40/0x50
>> [  174.530089]  remove_vm_area+0x20/0x90
>> [  174.530724]  __vunmap+0x1d/0xc0
>> [  174.531346]  ? delete_object_full+0x13/0x20
>> [  174.531973]  vfree+0x40/0x80
>> [  174.532594]  set_backend_state+0x18a/0xa90
>> [  174.533221]  ? dwc_scan_descriptors+0x24d/0x430
>> [  174.533850]  ? kfree+0x5b/0xc0
>> [  174.534476]  ? xenbus_read+0x3d/0x50
>> [  174.535101]  ? xenbus_read+0x3d/0x50
>> [  174.535718]  ? xenbus_gather+0x31/0x90
>> [  174.536332]  ? ___might_sleep+0xf6/0x130
>> [  174.536945]  frontend_changed+0x6b/0xd0
>> [  174.537565]  xenbus_otherend_changed+0x7d/0x80
>> [  174.538185]  frontend_changed+0x12/0x20
>> [  174.538803]  xenwatch_thread+0x74/0x110
>> [  174.539417]  ? woken_wake_function+0x20/0x20
>> [  174.540049]  kthread+0xe5/0x120
>> [  174.540663]  ? xenbus_printf+0x50/0x50
>> [  174.541278]  ? __kthread_init_worker+0x40/0x40
>> [  174.541898]  ret_from_fork+0x21/0x2c
>> [  174.548635] switch: port 2(vif3.0) entered disabled state
>>
>> I believe calling vfree() when holding a spin_lock isn't a good idea.
>>
> 
> Use vfree_atomic instead?

Hmm, isn't this overkill here?

You can just set a local variable with the address and do vfree() after
releasing the lock.


Juergen

[PATCH v4 2/3] xen: modify xenstore watch event interface

[Juergen Gross]

Today a Xenstore watch event is delivered via a callback function
declared as:

void (*callback)(struct xenbus_watch *,
 const char **vec, unsigned int len);

As all watch events only ever come with two parameters (path and token)
changing the prototype to:

void (*callback)(struct xenbus_watch *,
 const char *path, const char *token);

is the natural thing to do.

Apply this change and adapt all users.

Cc: konrad.w...@oracle.com
Cc: roger@citrix.com
Cc: wei.l...@citrix.com
Cc: paul.durr...@citrix.com
Cc: netdev@vger.kernel.org

Signed-off-by: Juergen Gross <jgr...@suse.com>
Reviewed-by: Paul Durrant <paul.durr...@citrix.com>
Reviewed-by: Wei Liu <wei.l...@citrix.com>
Reviewed-by: Roger Pau Monné <roger@citrix.com>
Reviewed-by: Boris Ostrovsky <boris.ostrov...@oracle.com>
---
 drivers/block/xen-blkback/xenbus.c |  6 +++---
 drivers/net/xen-netback/xenbus.c   |  8 
 drivers/xen/cpu_hotplug.c  |  5 ++---
 drivers/xen/manage.c   |  6 +++---
 drivers/xen/xen-balloon.c  |  2 +-
 drivers/xen/xen-pciback/xenbus.c   |  2 +-
 drivers/xen/xenbus/xenbus.h|  6 +++---
 drivers/xen/xenbus/xenbus_client.c |  4 ++--
 drivers/xen/xenbus/xenbus_dev_frontend.c   | 21 -
 drivers/xen/xenbus/xenbus_probe.c  | 11 ---
 drivers/xen/xenbus/xenbus_probe_backend.c  |  8 
 drivers/xen/xenbus/xenbus_probe_frontend.c | 14 +++---
 drivers/xen/xenbus/xenbus_xs.c | 29 ++---
 include/xen/xenbus.h   |  6 +++---
 14 files changed, 59 insertions(+), 69 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c 
b/drivers/block/xen-blkback/xenbus.c
index 415e79b..8fe61b5 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -38,8 +38,8 @@ struct backend_info {
 static struct kmem_cache *xen_blkif_cachep;
 static void connect(struct backend_info *);
 static int connect_ring(struct backend_info *);
-static void backend_changed(struct xenbus_watch *, const char **,
-   unsigned int);
+static void backend_changed(struct xenbus_watch *, const char *,
+   const char *);
 static void xen_blkif_free(struct xen_blkif *blkif);
 static void xen_vbd_free(struct xen_vbd *vbd);
 
@@ -661,7 +661,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
  * ready, connect.
  */
 static void backend_changed(struct xenbus_watch *watch,
-   const char **vec, unsigned int len)
+   const char *path, const char *token)
 {
int err;
unsigned major;
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 85b742e..bb854f9 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -734,7 +734,7 @@ static int xen_net_read_mac(struct xenbus_device *dev, u8 
mac[])
 }
 
 static void xen_net_rate_changed(struct xenbus_watch *watch,
-   const char **vec, unsigned int len)
+const char *path, const char *token)
 {
struct xenvif *vif = container_of(watch, struct xenvif, credit_watch);
struct xenbus_device *dev = xenvif_to_xenbus_device(vif);
@@ -791,7 +791,7 @@ static void xen_unregister_credit_watch(struct xenvif *vif)
 }
 
 static void xen_mcast_ctrl_changed(struct xenbus_watch *watch,
-  const char **vec, unsigned int len)
+  const char *path, const char *token)
 {
struct xenvif *vif = container_of(watch, struct xenvif,
  mcast_ctrl_watch);
@@ -866,8 +866,8 @@ static void unregister_hotplug_status_watch(struct 
backend_info *be)
 }
 
 static void hotplug_status_changed(struct xenbus_watch *watch,
-  const char **vec,
-  unsigned int vec_size)
+  const char *path,
+  const char *token)
 {
struct backend_info *be = container_of(watch,
   struct backend_info,
diff --git a/drivers/xen/cpu_hotplug.c b/drivers/xen/cpu_hotplug.c
index 5676aef..7a4daa2 100644
--- a/drivers/xen/cpu_hotplug.c
+++ b/drivers/xen/cpu_hotplug.c
@@ -68,13 +68,12 @@ static void vcpu_hotplug(unsigned int cpu)
 }
 
 static void handle_vcpu_hotplug_event(struct xenbus_watch *watch,
-   const char **vec, unsigned int len)
+ const char *path, const char *token)
 {
unsigned int cpu;
char *cpustr;
-   const char *node = vec[XS_WATCH_PATH];
 
-   cpustr = strstr(node, "cpu/");
+   cpustr = strstr(path, "cpu/");
if (cpustr

Re: [PATCH] xen-netfront: Delete rx_refill_timer in xennet_disconnect_backend()

[Juergen Gross]

On 30/01/17 18:45, Boris Ostrovsky wrote:
> rx_refill_timer should be deleted as soon as we disconnect from the
> backend since otherwise it is possible for the timer to go off before
> we get to xennet_destroy_queues(). If this happens we may dereference
> queue->rx.sring which is set to NULL in xennet_disconnect_backend().
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrov...@oracle.com>
> CC: sta...@vger.kernel.org

Reviewed-by: Juergen Gross <jgr...@suse.com>


Juergen

[PATCH v3 2/3] xen: modify xenstore watch event interface

[Juergen Gross]

Today a Xenstore watch event is delivered via a callback function
declared as:

void (*callback)(struct xenbus_watch *,
 const char **vec, unsigned int len);

As all watch events only ever come with two parameters (path and token)
changing the prototype to:

void (*callback)(struct xenbus_watch *,
 const char *path, const char *token);

is the natural thing to do.

Apply this change and adapt all users.

Cc: konrad.w...@oracle.com
Cc: roger@citrix.com
Cc: wei.l...@citrix.com
Cc: paul.durr...@citrix.com
Cc: netdev@vger.kernel.org

Signed-off-by: Juergen Gross <jgr...@suse.com>
Reviewed-by: Paul Durrant <paul.durr...@citrix.com>
Reviewed-by: Wei Liu <wei.l...@citrix.com>
Reviewed-by: Roger Pau Monné <roger@citrix.com>
Reviewed-by: Boris Ostrovsky <boris.ostrov...@oracle.com>
---
 drivers/block/xen-blkback/xenbus.c |  6 +++---
 drivers/net/xen-netback/xenbus.c   |  8 
 drivers/xen/cpu_hotplug.c  |  5 ++---
 drivers/xen/manage.c   |  6 +++---
 drivers/xen/xen-balloon.c  |  2 +-
 drivers/xen/xen-pciback/xenbus.c   |  2 +-
 drivers/xen/xenbus/xenbus.h|  6 +++---
 drivers/xen/xenbus/xenbus_client.c |  4 ++--
 drivers/xen/xenbus/xenbus_dev_frontend.c   | 21 -
 drivers/xen/xenbus/xenbus_probe.c  | 11 ---
 drivers/xen/xenbus/xenbus_probe_backend.c  |  8 
 drivers/xen/xenbus/xenbus_probe_frontend.c | 14 +++---
 drivers/xen/xenbus/xenbus_xs.c | 29 ++---
 include/xen/xenbus.h   |  6 +++---
 14 files changed, 59 insertions(+), 69 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c 
b/drivers/block/xen-blkback/xenbus.c
index 415e79b..8fe61b5 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -38,8 +38,8 @@ struct backend_info {
 static struct kmem_cache *xen_blkif_cachep;
 static void connect(struct backend_info *);
 static int connect_ring(struct backend_info *);
-static void backend_changed(struct xenbus_watch *, const char **,
-   unsigned int);
+static void backend_changed(struct xenbus_watch *, const char *,
+   const char *);
 static void xen_blkif_free(struct xen_blkif *blkif);
 static void xen_vbd_free(struct xen_vbd *vbd);
 
@@ -661,7 +661,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
  * ready, connect.
  */
 static void backend_changed(struct xenbus_watch *watch,
-   const char **vec, unsigned int len)
+   const char *path, const char *token)
 {
int err;
unsigned major;
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 3124eae..d8a40fa 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -723,7 +723,7 @@ static int xen_net_read_mac(struct xenbus_device *dev, u8 
mac[])
 }
 
 static void xen_net_rate_changed(struct xenbus_watch *watch,
-   const char **vec, unsigned int len)
+const char *path, const char *token)
 {
struct xenvif *vif = container_of(watch, struct xenvif, credit_watch);
struct xenbus_device *dev = xenvif_to_xenbus_device(vif);
@@ -780,7 +780,7 @@ static void xen_unregister_credit_watch(struct xenvif *vif)
 }
 
 static void xen_mcast_ctrl_changed(struct xenbus_watch *watch,
-  const char **vec, unsigned int len)
+  const char *path, const char *token)
 {
struct xenvif *vif = container_of(watch, struct xenvif,
  mcast_ctrl_watch);
@@ -855,8 +855,8 @@ static void unregister_hotplug_status_watch(struct 
backend_info *be)
 }
 
 static void hotplug_status_changed(struct xenbus_watch *watch,
-  const char **vec,
-  unsigned int vec_size)
+  const char *path,
+  const char *token)
 {
struct backend_info *be = container_of(watch,
   struct backend_info,
diff --git a/drivers/xen/cpu_hotplug.c b/drivers/xen/cpu_hotplug.c
index 5676aef..7a4daa2 100644
--- a/drivers/xen/cpu_hotplug.c
+++ b/drivers/xen/cpu_hotplug.c
@@ -68,13 +68,12 @@ static void vcpu_hotplug(unsigned int cpu)
 }
 
 static void handle_vcpu_hotplug_event(struct xenbus_watch *watch,
-   const char **vec, unsigned int len)
+ const char *path, const char *token)
 {
unsigned int cpu;
char *cpustr;
-   const char *node = vec[XS_WATCH_PATH];
 
-   cpustr = strstr(node, "cpu/");
+   cpustr = strstr(path, "cpu/");
if (cpustr

[PATCH v3 0/3] xen: optimize xenbus performance

[Juergen Gross]

The xenbus driver used for communication with Xenstore (all kernel
accesses to Xenstore and in case of Xenstore living in another domain
all accesses of the local domain to Xenstore) is rather simple
especially regarding multiple concurrent accesses: they are just being
serialized in spite of Xenstore being capable to handle multiple
parallel accesses.

Clean up the external interface(s) of xenbus and optimize its
performance by allowing multiple concurrent accesses to Xenstore.

V3:
- patch 3: simplify coding as requested by Boris Ostrovsky

V2:
- patch 1: update commit message, re-add lost copyright
- patch 3: address comments of Boris Ostrovsky:
- guard xs_request_id by lock
- move state struct definitions to the place where they are being
  used
- rate limit some warnings
- use barrier() instead of cpu_relax()
- add/remove some comments
- minor changes like naming of variables

Juergen Gross (3):
  xen: clean up xenbus internal headers
  xen: modify xenstore watch event interface
  xen: optimize xenbus driver for multiple concurrent xenstore accesses

 drivers/block/xen-blkback/xenbus.c |   6 +-
 drivers/net/xen-netback/xenbus.c   |   8 +-
 drivers/xen/cpu_hotplug.c  |   5 +-
 drivers/xen/manage.c   |   6 +-
 drivers/xen/xen-balloon.c  |   2 +-
 drivers/xen/xen-pciback/xenbus.c   |   2 +-
 drivers/xen/xenbus/xenbus.h| 135 +++
 drivers/xen/xenbus/xenbus_client.c |   6 +-
 drivers/xen/xenbus/xenbus_comms.c  | 315 +++--
 drivers/xen/xenbus/xenbus_comms.h  |  51 ---
 drivers/xen/xenbus/xenbus_dev_backend.c|   2 +-
 drivers/xen/xenbus/xenbus_dev_frontend.c   | 213 ++-
 drivers/xen/xenbus/xenbus_probe.c  |  14 +-
 drivers/xen/xenbus/xenbus_probe.h  |  88 -
 drivers/xen/xenbus/xenbus_probe_backend.c  |  11 +-
 drivers/xen/xenbus/xenbus_probe_frontend.c |  17 +-
 drivers/xen/xenbus/xenbus_xs.c | 544 +
 drivers/xen/xenfs/super.c  |   2 +-
 drivers/xen/xenfs/xenstored.c  |   2 +-
 include/xen/xenbus.h   |  18 +-
 20 files changed, 835 insertions(+), 612 deletions(-)
 create mode 100644 drivers/xen/xenbus/xenbus.h
 delete mode 100644 drivers/xen/xenbus/xenbus_comms.h
 delete mode 100644 drivers/xen/xenbus/xenbus_probe.h

Cc: konrad.w...@oracle.com
Cc: roger@citrix.com
Cc: wei.l...@citrix.com
Cc: paul.durr...@citrix.com
Cc: netdev@vger.kernel.org
-- 
2.10.2

[PATCH v2 2/3] xen: modify xenstore watch event interface

[Juergen Gross]

Today a Xenstore watch event is delivered via a callback function
declared as:

void (*callback)(struct xenbus_watch *,
 const char **vec, unsigned int len);

As all watch events only ever come with two parameters (path and token)
changing the prototype to:

void (*callback)(struct xenbus_watch *,
 const char *path, const char *token);

is the natural thing to do.

Apply this change and adapt all users.

Cc: konrad.w...@oracle.com
Cc: roger@citrix.com
Cc: wei.l...@citrix.com
Cc: paul.durr...@citrix.com
Cc: netdev@vger.kernel.org

Signed-off-by: Juergen Gross <jgr...@suse.com>
Reviewed-by: Paul Durrant <paul.durr...@citrix.com>
Reviewed-by: Wei Liu <wei.l...@citrix.com>
Reviewed-by: Roger Pau Monné <roger@citrix.com>
Reviewed-by: Boris Ostrovsky <boris.ostrov...@oracle.com>
---
 drivers/block/xen-blkback/xenbus.c |  6 +++---
 drivers/net/xen-netback/xenbus.c   |  8 
 drivers/xen/cpu_hotplug.c  |  5 ++---
 drivers/xen/manage.c   |  6 +++---
 drivers/xen/xen-balloon.c  |  2 +-
 drivers/xen/xen-pciback/xenbus.c   |  2 +-
 drivers/xen/xenbus/xenbus.h|  6 +++---
 drivers/xen/xenbus/xenbus_client.c |  4 ++--
 drivers/xen/xenbus/xenbus_dev_frontend.c   | 21 -
 drivers/xen/xenbus/xenbus_probe.c  | 11 ---
 drivers/xen/xenbus/xenbus_probe_backend.c  |  8 
 drivers/xen/xenbus/xenbus_probe_frontend.c | 14 +++---
 drivers/xen/xenbus/xenbus_xs.c | 29 ++---
 include/xen/xenbus.h   |  6 +++---
 14 files changed, 59 insertions(+), 69 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c 
b/drivers/block/xen-blkback/xenbus.c
index 415e79b..8fe61b5 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -38,8 +38,8 @@ struct backend_info {
 static struct kmem_cache *xen_blkif_cachep;
 static void connect(struct backend_info *);
 static int connect_ring(struct backend_info *);
-static void backend_changed(struct xenbus_watch *, const char **,
-   unsigned int);
+static void backend_changed(struct xenbus_watch *, const char *,
+   const char *);
 static void xen_blkif_free(struct xen_blkif *blkif);
 static void xen_vbd_free(struct xen_vbd *vbd);
 
@@ -661,7 +661,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
  * ready, connect.
  */
 static void backend_changed(struct xenbus_watch *watch,
-   const char **vec, unsigned int len)
+   const char *path, const char *token)
 {
int err;
unsigned major;
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 3124eae..d8a40fa 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -723,7 +723,7 @@ static int xen_net_read_mac(struct xenbus_device *dev, u8 
mac[])
 }
 
 static void xen_net_rate_changed(struct xenbus_watch *watch,
-   const char **vec, unsigned int len)
+const char *path, const char *token)
 {
struct xenvif *vif = container_of(watch, struct xenvif, credit_watch);
struct xenbus_device *dev = xenvif_to_xenbus_device(vif);
@@ -780,7 +780,7 @@ static void xen_unregister_credit_watch(struct xenvif *vif)
 }
 
 static void xen_mcast_ctrl_changed(struct xenbus_watch *watch,
-  const char **vec, unsigned int len)
+  const char *path, const char *token)
 {
struct xenvif *vif = container_of(watch, struct xenvif,
  mcast_ctrl_watch);
@@ -855,8 +855,8 @@ static void unregister_hotplug_status_watch(struct 
backend_info *be)
 }
 
 static void hotplug_status_changed(struct xenbus_watch *watch,
-  const char **vec,
-  unsigned int vec_size)
+  const char *path,
+  const char *token)
 {
struct backend_info *be = container_of(watch,
   struct backend_info,
diff --git a/drivers/xen/cpu_hotplug.c b/drivers/xen/cpu_hotplug.c
index 5676aef..7a4daa2 100644
--- a/drivers/xen/cpu_hotplug.c
+++ b/drivers/xen/cpu_hotplug.c
@@ -68,13 +68,12 @@ static void vcpu_hotplug(unsigned int cpu)
 }
 
 static void handle_vcpu_hotplug_event(struct xenbus_watch *watch,
-   const char **vec, unsigned int len)
+ const char *path, const char *token)
 {
unsigned int cpu;
char *cpustr;
-   const char *node = vec[XS_WATCH_PATH];
 
-   cpustr = strstr(node, "cpu/");
+   cpustr = strstr(path, "cpu/");
if (cpustr

[PATCH v2 0/3] xen: optimize xenbus performance

[Juergen Gross]

The xenbus driver used for communication with Xenstore (all kernel
accesses to Xenstore and in case of Xenstore living in another domain
all accesses of the local domain to Xenstore) is rather simple
especially regarding multiple concurrent accesses: they are just being
serialized in spite of Xenstore being capable to handle multiple
parallel accesses.

Clean up the external interface(s) of xenbus and optimize its
performance by allowing multiple concurrent accesses to Xenstore.

V2:
- patch 1: update commit message, re-add lost copyright
- patch 3: address comments of Boris Ostrovsky:
- guard xs_request_id by lock
- move state struct definitions to the place where they are being
  used
- rate limit some warnings
- use barrier() instead of cpu_relax()
- add/remove some comments
- minor changes like naming of variables

Juergen Gross (3):
  xen: clean up xenbus internal headers
  xen: modify xenstore watch event interface
  xen: optimize xenbus driver for multiple concurrent xenstore accesses

 drivers/block/xen-blkback/xenbus.c |   6 +-
 drivers/net/xen-netback/xenbus.c   |   8 +-
 drivers/xen/cpu_hotplug.c  |   5 +-
 drivers/xen/manage.c   |   6 +-
 drivers/xen/xen-balloon.c  |   2 +-
 drivers/xen/xen-pciback/xenbus.c   |   2 +-
 drivers/xen/xenbus/xenbus.h| 135 +++
 drivers/xen/xenbus/xenbus_client.c |   6 +-
 drivers/xen/xenbus/xenbus_comms.c  | 315 +++--
 drivers/xen/xenbus/xenbus_comms.h  |  51 ---
 drivers/xen/xenbus/xenbus_dev_backend.c|   2 +-
 drivers/xen/xenbus/xenbus_dev_frontend.c   | 213 ++-
 drivers/xen/xenbus/xenbus_probe.c  |  14 +-
 drivers/xen/xenbus/xenbus_probe.h  |  88 -
 drivers/xen/xenbus/xenbus_probe_backend.c  |  11 +-
 drivers/xen/xenbus/xenbus_probe_frontend.c |  17 +-
 drivers/xen/xenbus/xenbus_xs.c | 547 +
 drivers/xen/xenfs/super.c  |   2 +-
 drivers/xen/xenfs/xenstored.c  |   2 +-
 include/xen/xenbus.h   |  18 +-
 20 files changed, 838 insertions(+), 612 deletions(-)
 create mode 100644 drivers/xen/xenbus/xenbus.h
 delete mode 100644 drivers/xen/xenbus/xenbus_comms.h
 delete mode 100644 drivers/xen/xenbus/xenbus_probe.h

Cc: konrad.w...@oracle.com
Cc: roger@citrix.com
Cc: wei.l...@citrix.com
Cc: paul.durr...@citrix.com
Cc: netdev@vger.kernel.org
-- 
2.10.2

Re: [PATCH v2] xen-netfront: Fix Rx stall during network stress and OOM

[Juergen Gross]

On 13/01/17 18:55, Remanan Pillai wrote:
> From: Vineeth Remanan Pillai <vinee...@amazon.com>
> 
> During an OOM scenario, request slots could not be created as skb
> allocation fails. So the netback cannot pass in packets and netfront
> wrongly assumes that there is no more work to be done and it disables
> polling. This causes Rx to stall.
> 
> The issue is with the retry logic which schedules the timer if the
> created slots are less than NET_RX_SLOTS_MIN. The count of new request
> slots to be pushed are calculated as a difference between new req_prod
> and rsp_cons which could be more than the actual slots, if there are
> unconsumed responses.
> 
> The fix is to calculate the count of newly created slots as the
> difference between new req_prod and old req_prod.
> 
> Signed-off-by: Vineeth Remanan Pillai <vinee...@amazon.com>

Reviewed-by: Juergen Gross <jgr...@suse.com>


Thanks,

Juergen

Re: [PATCH 1/2] xen/netfront: set default upper limit of tx/rx queues to 8

[Juergen Gross]

On 10/01/17 15:53, Boris Ostrovsky wrote:
> On 01/10/2017 08:32 AM, Juergen Gross wrote:
>> The default for the number of tx/rx queues of one interface is the
>> number of vcpus of the system today. As each queue pair reserves 512
>> grant pages this default consumes a ridiculous number of grants for
>> large guests.
>>
>> Limit the queue number to 8 as default. This value can be modified
>> via a module parameter if required.
>>
>> Signed-off-by: Juergen Gross <jgr...@suse.com>
>> ---
>>  drivers/net/xen-netfront.c | 6 --
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
>> index a479cd9..490c865 100644
>> --- a/drivers/net/xen-netfront.c
>> +++ b/drivers/net/xen-netfront.c
>> @@ -57,6 +57,7 @@
>>  #include 
>>  
>>  /* Module parameters */
>> +#define MAX_QUEUES_DEFAULT 8
>>  static unsigned int xennet_max_queues;
>>  module_param_named(max_queues, xennet_max_queues, uint, 0644);
>>  MODULE_PARM_DESC(max_queues,
>> @@ -2164,11 +2165,12 @@ static int __init netif_init(void)
>>  
>>  pr_info("Initialising Xen virtual ethernet driver\n");
>>  
>> -/* Allow as many queues as there are CPUs if user has not
>> +/* Allow as many queues as there are CPUs inut max. 8 if user has not
> 
> Based on comment change in the second patch: s/inut/but/ ? Also, comment
> style in both patches.

Typo: yes
Style: no. checkpatch tells me that the net directory doesn't follow
common kernel style.

> 
> Other than that, for both:
> 
> Reviewed-by: Boris Ostrovsky <boris.ostrov...@oracle.com>

Thanks,

Juergen

[PATCH 1/2] xen/netfront: set default upper limit of tx/rx queues to 8

[Juergen Gross]

The default for the number of tx/rx queues of one interface is the
number of vcpus of the system today. As each queue pair reserves 512
grant pages this default consumes a ridiculous number of grants for
large guests.

Limit the queue number to 8 as default. This value can be modified
via a module parameter if required.

Signed-off-by: Juergen Gross <jgr...@suse.com>
---
 drivers/net/xen-netfront.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index a479cd9..490c865 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -57,6 +57,7 @@
 #include 
 
 /* Module parameters */
+#define MAX_QUEUES_DEFAULT 8
 static unsigned int xennet_max_queues;
 module_param_named(max_queues, xennet_max_queues, uint, 0644);
 MODULE_PARM_DESC(max_queues,
@@ -2164,11 +2165,12 @@ static int __init netif_init(void)
 
pr_info("Initialising Xen virtual ethernet driver\n");
 
-   /* Allow as many queues as there are CPUs if user has not
+   /* Allow as many queues as there are CPUs inut max. 8 if user has not
 * specified a value.
 */
if (xennet_max_queues == 0)
-   xennet_max_queues = num_online_cpus();
+   xennet_max_queues = min_t(unsigned int, MAX_QUEUES_DEFAULT,
+ num_online_cpus());
 
return xenbus_register_frontend(_driver);
 }
-- 
2.10.2

[PATCH 0/2] xen/net: limit number of tx/rx queues

[Juergen Gross]

The Xen network frontend/backend supports multiple tx/rx queues for one
virtual interface. The number of queues supported by the backend is
set to the number of cpus of the backend driver domain (usually dom0)
and the number of queues requested by the frontend is limited by the
number of vcpus of the related guest.

On large systems this can lead to ridiculous large number of queues
exhausting the required number of grant pages rather quick.

To avoid this limit the default maximum on both sides to 8. Both
frontend and backend maximum can be individually tuned via module
parameters.

Juergen Gross (2):
  xen/netfront: set default upper limit of tx/rx queues to 8
  xen/netback: set default upper limit of tx/rx queues to 8

 drivers/net/xen-netback/netback.c | 6 --
 drivers/net/xen-netfront.c| 6 --
 2 files changed, 8 insertions(+), 4 deletions(-)

-- 
2.10.2

[PATCH 2/2] xen/netback: set default upper limit of tx/rx queues to 8

[Juergen Gross]

The default for the maximum number of tx/rx queues of one interface is
the number of cpus of the system today. As each queue pair reserves 512
grant pages this default consumes a ridiculous number of grants for
large guests.

Limit the queue number to 8 as default. This value can be modified
via a module parameter if required.

Signed-off-by: Juergen Gross <jgr...@suse.com>
---
 drivers/net/xen-netback/netback.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c 
b/drivers/net/xen-netback/netback.c
index 47b4810..f9bcf4a 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -67,6 +67,7 @@ module_param(rx_drain_timeout_msecs, uint, 0444);
 unsigned int rx_stall_timeout_msecs = 6;
 module_param(rx_stall_timeout_msecs, uint, 0444);
 
+#define MAX_QUEUES_DEFAULT 8
 unsigned int xenvif_max_queues;
 module_param_named(max_queues, xenvif_max_queues, uint, 0644);
 MODULE_PARM_DESC(max_queues,
@@ -1622,11 +1623,12 @@ static int __init netback_init(void)
if (!xen_domain())
return -ENODEV;
 
-   /* Allow as many queues as there are CPUs if user has not
+   /* Allow as many queues as there are CPUs but max. 8 if user has not
 * specified a value.
 */
if (xenvif_max_queues == 0)
-   xenvif_max_queues = num_online_cpus();
+   xenvif_max_queues = min_t(unsigned int, MAX_QUEUES_DEFAULT,
+ num_online_cpus());
 
if (fatal_skb_slots < XEN_NETBK_LEGACY_SLOTS_MAX) {
pr_info("fatal_skb_slots too small (%d), bump it to 
XEN_NETBK_LEGACY_SLOTS_MAX (%d)\n",
-- 
2.10.2

Re: [PATCH 2/3] xen: modify xenstore watch event interface

[Juergen Gross]

On 06/01/17 22:57, Boris Ostrovsky wrote:
> On 01/06/2017 10:05 AM, Juergen Gross wrote:
>> Today a Xenstore watch event is delivered via a callback function
>> declared as:
>>
>> void (*callback)(struct xenbus_watch *,
>>  const char **vec, unsigned int len);
>>
>> As all watch events only ever come with two parameters (path and token)
>> changing the prototype to:
>>
>> void (*callback)(struct xenbus_watch *,
>>  const char *path, const char *token);
>>
>> is the natural thing to do.
>>
>> Apply this change and adapt all users.
>>
>> Cc: konrad.w...@oracle.com
>> Cc: roger@citrix.com
>> Cc: wei.l...@citrix.com
>> Cc: paul.durr...@citrix.com
>> Cc: netdev@vger.kernel.org
>>
>> Signed-off-by: Juergen Gross <jgr...@suse.com>
> 
> 
>>  
>> @@ -903,24 +902,24 @@ static int process_msg(void)
>>  body[msg->hdr.len] = '\0';
>>  
>>  if (msg->hdr.type == XS_WATCH_EVENT) {
>> -msg->u.watch.vec = split(body, msg->hdr.len,
>> - >u.watch.vec_size);
>> -if (IS_ERR(msg->u.watch.vec)) {
>> -err = PTR_ERR(msg->u.watch.vec);
>> +if (count_strings(body, msg->hdr.len) != 2) {
>> +err = -EINVAL;
> 
> xenbus_write_watch() returns -EILSEQ when this type of error is
> encountered so perhaps for we should return the same error here.

Not since 9a6161fe73bdd3ae4a1e18421b0b20cb7141f680. :-)

> 
> Either way
> 
> Reviewed-by: Boris Ostrovsky <boris.ostrov...@oracle.com>

Thanks,

Juergen

[PATCH 0/3] xen: optimize xenbus performance

[Juergen Gross]

The xenbus driver used for communication with Xenstore (all kernel
accesses to Xenstore and in case of Xenstore living in another domain
all accesses of the local domain to Xenstore) is rather simple
especially regarding multiple concurrent accesses: they are just being
serialized in spite of Xenstore being capable to handle multiple
parallel accesses.

Clean up the external interface(s) of xenbus and optimize its
performance by allowing multiple concurrent accesses to Xenstore.

Juergen Gross (3):
  xen: clean up xenbus internal headers
  xen: modify xenstore watch event interface
  xen: optimize xenbus driver for multiple concurrent xenstore accesses

 drivers/block/xen-blkback/xenbus.c |   6 +-
 drivers/net/xen-netback/xenbus.c   |   8 +-
 drivers/xen/cpu_hotplug.c  |   5 +-
 drivers/xen/manage.c   |   6 +-
 drivers/xen/xen-balloon.c  |   2 +-
 drivers/xen/xen-pciback/xenbus.c   |   2 +-
 drivers/xen/xenbus/xenbus.h| 134 
 drivers/xen/xenbus/xenbus_client.c |   6 +-
 drivers/xen/xenbus/xenbus_comms.c  | 319 +++--
 drivers/xen/xenbus/xenbus_comms.h  |  51 ---
 drivers/xen/xenbus/xenbus_dev_backend.c|   2 +-
 drivers/xen/xenbus/xenbus_dev_frontend.c   | 213 +++-
 drivers/xen/xenbus/xenbus_probe.c  |  14 +-
 drivers/xen/xenbus/xenbus_probe.h  |  88 -
 drivers/xen/xenbus/xenbus_probe_backend.c  |  11 +-
 drivers/xen/xenbus/xenbus_probe_frontend.c |  17 +-
 drivers/xen/xenbus/xenbus_xs.c | 535 +
 drivers/xen/xenfs/super.c  |   2 +-
 drivers/xen/xenfs/xenstored.c  |   2 +-
 include/xen/xenbus.h   |  18 +-
 20 files changed, 830 insertions(+), 611 deletions(-)
 create mode 100644 drivers/xen/xenbus/xenbus.h
 delete mode 100644 drivers/xen/xenbus/xenbus_comms.h
 delete mode 100644 drivers/xen/xenbus/xenbus_probe.h

Cc: konrad.w...@oracle.com
Cc: roger@citrix.com
Cc: wei.l...@citrix.com
Cc: paul.durr...@citrix.com
Cc: netdev@vger.kernel.org

-- 
2.10.2

[PATCH 2/3] xen: modify xenstore watch event interface

[Juergen Gross]

Today a Xenstore watch event is delivered via a callback function
declared as:

void (*callback)(struct xenbus_watch *,
 const char **vec, unsigned int len);

As all watch events only ever come with two parameters (path and token)
changing the prototype to:

void (*callback)(struct xenbus_watch *,
 const char *path, const char *token);

is the natural thing to do.

Apply this change and adapt all users.

Cc: konrad.w...@oracle.com
Cc: roger@citrix.com
Cc: wei.l...@citrix.com
Cc: paul.durr...@citrix.com
Cc: netdev@vger.kernel.org

Signed-off-by: Juergen Gross <jgr...@suse.com>
---
 drivers/block/xen-blkback/xenbus.c |  6 +++---
 drivers/net/xen-netback/xenbus.c   |  8 
 drivers/xen/cpu_hotplug.c  |  5 ++---
 drivers/xen/manage.c   |  6 +++---
 drivers/xen/xen-balloon.c  |  2 +-
 drivers/xen/xen-pciback/xenbus.c   |  2 +-
 drivers/xen/xenbus/xenbus.h|  6 +++---
 drivers/xen/xenbus/xenbus_client.c |  4 ++--
 drivers/xen/xenbus/xenbus_dev_frontend.c   | 21 -
 drivers/xen/xenbus/xenbus_probe.c  | 11 ---
 drivers/xen/xenbus/xenbus_probe_backend.c  |  8 
 drivers/xen/xenbus/xenbus_probe_frontend.c | 14 +++---
 drivers/xen/xenbus/xenbus_xs.c | 29 ++---
 include/xen/xenbus.h   |  6 +++---
 14 files changed, 59 insertions(+), 69 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c 
b/drivers/block/xen-blkback/xenbus.c
index 415e79b..8fe61b5 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -38,8 +38,8 @@ struct backend_info {
 static struct kmem_cache *xen_blkif_cachep;
 static void connect(struct backend_info *);
 static int connect_ring(struct backend_info *);
-static void backend_changed(struct xenbus_watch *, const char **,
-   unsigned int);
+static void backend_changed(struct xenbus_watch *, const char *,
+   const char *);
 static void xen_blkif_free(struct xen_blkif *blkif);
 static void xen_vbd_free(struct xen_vbd *vbd);
 
@@ -661,7 +661,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
  * ready, connect.
  */
 static void backend_changed(struct xenbus_watch *watch,
-   const char **vec, unsigned int len)
+   const char *path, const char *token)
 {
int err;
unsigned major;
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 3124eae..d8a40fa 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -723,7 +723,7 @@ static int xen_net_read_mac(struct xenbus_device *dev, u8 
mac[])
 }
 
 static void xen_net_rate_changed(struct xenbus_watch *watch,
-   const char **vec, unsigned int len)
+const char *path, const char *token)
 {
struct xenvif *vif = container_of(watch, struct xenvif, credit_watch);
struct xenbus_device *dev = xenvif_to_xenbus_device(vif);
@@ -780,7 +780,7 @@ static void xen_unregister_credit_watch(struct xenvif *vif)
 }
 
 static void xen_mcast_ctrl_changed(struct xenbus_watch *watch,
-  const char **vec, unsigned int len)
+  const char *path, const char *token)
 {
struct xenvif *vif = container_of(watch, struct xenvif,
  mcast_ctrl_watch);
@@ -855,8 +855,8 @@ static void unregister_hotplug_status_watch(struct 
backend_info *be)
 }
 
 static void hotplug_status_changed(struct xenbus_watch *watch,
-  const char **vec,
-  unsigned int vec_size)
+  const char *path,
+  const char *token)
 {
struct backend_info *be = container_of(watch,
   struct backend_info,
diff --git a/drivers/xen/cpu_hotplug.c b/drivers/xen/cpu_hotplug.c
index 5676aef..7a4daa2 100644
--- a/drivers/xen/cpu_hotplug.c
+++ b/drivers/xen/cpu_hotplug.c
@@ -68,13 +68,12 @@ static void vcpu_hotplug(unsigned int cpu)
 }
 
 static void handle_vcpu_hotplug_event(struct xenbus_watch *watch,
-   const char **vec, unsigned int len)
+ const char *path, const char *token)
 {
unsigned int cpu;
char *cpustr;
-   const char *node = vec[XS_WATCH_PATH];
 
-   cpustr = strstr(node, "cpu/");
+   cpustr = strstr(path, "cpu/");
if (cpustr != NULL) {
sscanf(cpustr, "cpu/%u", );
vcpu_hotplug(cpu);
diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 26e5e85..ca62c09 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage

Re: [Xen PATCH] xen-netback: fix error handling output

[Juergen Gross]

On 08/11/16 14:34, Arnd Bergmann wrote:
> The connect function prints an unintialized error code after an
> earlier initialization was removed:
> 
> drivers/net/xen-netback/xenbus.c: In function 'connect':
> drivers/net/xen-netback/xenbus.c:938:3: error: 'err' may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
> 
> This prints it as -EINVAL instead, which seems to be the most
> appropriate error code. Before the patch that caused the warning,
> this would print a positive number returned by vsscanf() instead,
> which is also wrong. We probably don't need a backport though,
> as fixing the warning here should be sufficient.
> 
> Fixes: f95842e7a9f2 ("xen: make use of xenbus_read_unsigned() in xen-netback")
> Fixes: 8d3d53b3e433 ("xen-netback: Add support for multiple queues")
> Signed-off-by: Arnd Bergmann 

Applied to xen/tip.git for-linus-4.10


Thanks,

Juergen

> ---
>  drivers/net/xen-netback/xenbus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/xen-netback/xenbus.c 
> b/drivers/net/xen-netback/xenbus.c
> index 7356e00fac54..bfed79877b8a 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -935,7 +935,7 @@ static void connect(struct backend_info *be)
>   "multi-queue-num-queues", 1);
>   if (requested_num_queues > xenvif_max_queues) {
>   /* buggy or malicious guest */
> - xenbus_dev_fatal(dev, err,
> + xenbus_dev_fatal(dev, -EINVAL,
>"guest requested %u queues, exceeding the 
> maximum of %u.",
>requested_num_queues, xenvif_max_queues);
>   return;
> 

Re: [PATCH 00/12] xen: add common function for reading optional value

[Juergen Gross]

On 31/10/16 18:08, David Miller wrote:
> From: Juergen Gross <jgr...@suse.com>
> Date: Mon, 31 Oct 2016 17:48:18 +0100
> 
>> There are multiple instances of code reading an optional unsigned
>> parameter from Xenstore via xenbus_scanf(). Instead of repeating the
>> same code over and over add a service function doing the job and
>> replace the call of xenbus_scanf() with the call of the new function
>> where appropriate.
> 
> As this seems to be a series that will go through some tree other
> than mine, I assume the networking bits will be taken care of that
> way.
> 

If accepted I expect this series to go through the Xen tree.


Juergen

[PATCH 07/12] xen: make use of xenbus_read_unsigned() in xen-netfront

[Juergen Gross]

Use xenbus_read_unsigned() instead of xenbus_scanf() when possible.
This requires to change the type of some reads from int to unsigned,
but these cases have been wrong before: negative values are not allowed
for the modified cases.

Cc: netdev@vger.kernel.org

Signed-off-by: Juergen Gross <jgr...@suse.com>
---
 drivers/net/xen-netfront.c | 67 +-
 1 file changed, 18 insertions(+), 49 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index e17879d..95d664e 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -1169,43 +1169,23 @@ static netdev_features_t xennet_fix_features(struct 
net_device *dev,
netdev_features_t features)
 {
struct netfront_info *np = netdev_priv(dev);
-   int val;
 
-   if (features & NETIF_F_SG) {
-   if (xenbus_scanf(XBT_NIL, np->xbdev->otherend, "feature-sg",
-"%d", ) < 0)
-   val = 0;
+   if (features & NETIF_F_SG &&
+   !xenbus_read_unsigned(np->xbdev->otherend, "feature-sg", 0))
+   features &= ~NETIF_F_SG;
 
-   if (!val)
-   features &= ~NETIF_F_SG;
-   }
+   if (features & NETIF_F_IPV6_CSUM &&
+   !xenbus_read_unsigned(np->xbdev->otherend,
+ "feature-ipv6-csum-offload", 0))
+   features &= ~NETIF_F_IPV6_CSUM;
 
-   if (features & NETIF_F_IPV6_CSUM) {
-   if (xenbus_scanf(XBT_NIL, np->xbdev->otherend,
-"feature-ipv6-csum-offload", "%d", ) < 0)
-   val = 0;
+   if (features & NETIF_F_TSO &&
+   !xenbus_read_unsigned(np->xbdev->otherend, "feature-gso-tcpv4", 0))
+   features &= ~NETIF_F_TSO;
 
-   if (!val)
-   features &= ~NETIF_F_IPV6_CSUM;
-   }
-
-   if (features & NETIF_F_TSO) {
-   if (xenbus_scanf(XBT_NIL, np->xbdev->otherend,
-"feature-gso-tcpv4", "%d", ) < 0)
-   val = 0;
-
-   if (!val)
-   features &= ~NETIF_F_TSO;
-   }
-
-   if (features & NETIF_F_TSO6) {
-   if (xenbus_scanf(XBT_NIL, np->xbdev->otherend,
-"feature-gso-tcpv6", "%d", ) < 0)
-   val = 0;
-
-   if (!val)
-   features &= ~NETIF_F_TSO6;
-   }
+   if (features & NETIF_F_TSO6 &&
+   !xenbus_read_unsigned(np->xbdev->otherend, "feature-gso-tcpv6", 0))
+   features &= ~NETIF_F_TSO6;
 
return features;
 }
@@ -1821,18 +1801,13 @@ static int talk_to_netback(struct xenbus_device *dev,
info->netdev->irq = 0;
 
/* Check if backend supports multiple queues */
-   err = xenbus_scanf(XBT_NIL, info->xbdev->otherend,
-  "multi-queue-max-queues", "%u", _queues);
-   if (err < 0)
-   max_queues = 1;
+   max_queues = xenbus_read_unsigned(info->xbdev->otherend,
+ "multi-queue-max-queues", 1);
num_queues = min(max_queues, xennet_max_queues);
 
/* Check feature-split-event-channels */
-   err = xenbus_scanf(XBT_NIL, info->xbdev->otherend,
-  "feature-split-event-channels", "%u",
-  _split_evtchn);
-   if (err < 0)
-   feature_split_evtchn = 0;
+   feature_split_evtchn = xenbus_read_unsigned(info->xbdev->otherend,
+   "feature-split-event-channels", 0);
 
/* Read mac addr. */
err = xen_net_read_mac(dev, info->netdev->dev_addr);
@@ -1966,16 +1941,10 @@ static int xennet_connect(struct net_device *dev)
struct netfront_info *np = netdev_priv(dev);
unsigned int num_queues = 0;
int err;
-   unsigned int feature_rx_copy;
unsigned int j = 0;
struct netfront_queue *queue = NULL;
 
-   err = xenbus_scanf(XBT_NIL, np->xbdev->otherend,
-  "feature-rx-copy", "%u", _rx_copy);
-   if (err != 1)
-   feature_rx_copy = 0;
-
-   if (!feature_rx_copy) {
+   if (!xenbus_read_unsigned(np->xbdev->otherend, "feature-rx-copy", 0)) {
dev_info(>dev,
 "backend does not support copying receive path\n");
return -ENODEV;
-- 
2.6.6

[PATCH 00/12] xen: add common function for reading optional value

[Juergen Gross]

There are multiple instances of code reading an optional unsigned
parameter from Xenstore via xenbus_scanf(). Instead of repeating the
same code over and over add a service function doing the job and
replace the call of xenbus_scanf() with the call of the new function
where appropriate.

Juergen Gross (12):
  xen: introduce xenbus_read_unsigned()
  xen: make use of xenbus_read_unsigned() in xen-blkback
  xen: make use of xenbus_read_unsigned() in xen-blkfront
  xen: make use of xenbus_read_unsigned() in xen-tpmfront
  xen: make use of xenbus_read_unsigned() in xen-kbdfront
  xen: make use of xenbus_read_unsigned() in xen-netback
  xen: make use of xenbus_read_unsigned() in xen-netfront
  xen: make use of xenbus_read_unsigned() in xen-pcifront
  xen: make use of xenbus_read_unsigned() in xen-scsifront
  xen: make use of xenbus_read_unsigned() in xen-fbfront
  xen: make use of xenbus_read_unsigned() in xen-pciback
  xen: make use of xenbus_read_unsigned() in xenbus

 drivers/block/xen-blkback/xenbus.c| 36 ++
 drivers/block/xen-blkfront.c  | 81 ++-
 drivers/char/tpm/xen-tpmfront.c   |  8 +--
 drivers/input/misc/xen-kbdfront.c | 13 ++---
 drivers/net/xen-netback/xenbus.c  | 50 ++-
 drivers/net/xen-netfront.c| 67 +++--
 drivers/pci/xen-pcifront.c|  6 +--
 drivers/scsi/xen-scsifront.c  |  6 +--
 drivers/video/fbdev/xen-fbfront.c | 13 ++---
 drivers/xen/xen-pciback/xenbus.c  |  8 ++-
 drivers/xen/xenbus/xenbus_probe_backend.c |  8 +--
 drivers/xen/xenbus/xenbus_xs.c| 22 +++--
 include/xen/xenbus.h  |  4 ++
 13 files changed, 112 insertions(+), 210 deletions(-)

Cc: konrad.w...@oracle.com
Cc: roger@citrix.com
Cc: peterhu...@gmx.de
Cc: tp...@selhorst.net
Cc: jarkko.sakki...@linux.intel.com
Cc: jguntho...@obsidianresearch.com
Cc: tpmdd-de...@lists.sourceforge.net
Cc: dmitry.torok...@gmail.com
Cc: linux-in...@vger.kernel.org
Cc: wei.l...@citrix.com
Cc: paul.durr...@citrix.com
Cc: netdev@vger.kernel.org
Cc: bhelg...@google.com
Cc: linux-...@vger.kernel.org
Cc: tomi.valkei...@ti.com
Cc: linux-fb...@vger.kernel.org
-- 
2.6.6

[PATCH 06/12] xen: make use of xenbus_read_unsigned() in xen-netback

[Juergen Gross]

Use xenbus_read_unsigned() instead of xenbus_scanf() when possible.
This requires to change the type of some reads from int to unsigned,
but these cases have been wrong before: negative values are not allowed
for the modified cases.

Cc: wei.l...@citrix.com
Cc: paul.durr...@citrix.com
Cc: netdev@vger.kernel.org

Signed-off-by: Juergen Gross <jgr...@suse.com>
---
 drivers/net/xen-netback/xenbus.c | 50 +++-
 1 file changed, 14 insertions(+), 36 deletions(-)

diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 8674e18..7356e00 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -785,12 +785,9 @@ static void xen_mcast_ctrl_changed(struct xenbus_watch 
*watch,
struct xenvif *vif = container_of(watch, struct xenvif,
  mcast_ctrl_watch);
struct xenbus_device *dev = xenvif_to_xenbus_device(vif);
-   int val;
 
-   if (xenbus_scanf(XBT_NIL, dev->otherend,
-"request-multicast-control", "%d", ) < 0)
-   val = 0;
-   vif->multicast_control = !!val;
+   vif->multicast_control = !!xenbus_read_unsigned(dev->otherend,
+   "request-multicast-control", 0);
 }
 
 static int xen_register_mcast_ctrl_watch(struct xenbus_device *dev,
@@ -934,12 +931,9 @@ static void connect(struct backend_info *be)
/* Check whether the frontend requested multiple queues
 * and read the number requested.
 */
-   err = xenbus_scanf(XBT_NIL, dev->otherend,
-  "multi-queue-num-queues",
-  "%u", _num_queues);
-   if (err < 0) {
-   requested_num_queues = 1; /* Fall back to single queue */
-   } else if (requested_num_queues > xenvif_max_queues) {
+   requested_num_queues = xenbus_read_unsigned(dev->otherend,
+   "multi-queue-num-queues", 1);
+   if (requested_num_queues > xenvif_max_queues) {
/* buggy or malicious guest */
xenbus_dev_fatal(dev, err,
 "guest requested %u queues, exceeding the 
maximum of %u.",
@@ -1134,7 +1128,7 @@ static int read_xenbus_vif_flags(struct backend_info *be)
struct xenvif *vif = be->vif;
struct xenbus_device *dev = be->dev;
unsigned int rx_copy;
-   int err, val;
+   int err;
 
err = xenbus_scanf(XBT_NIL, dev->otherend, "request-rx-copy", "%u",
   _copy);
@@ -1150,10 +1144,7 @@ static int read_xenbus_vif_flags(struct backend_info *be)
if (!rx_copy)
return -EOPNOTSUPP;
 
-   if (xenbus_scanf(XBT_NIL, dev->otherend,
-"feature-rx-notify", "%d", ) < 0)
-   val = 0;
-   if (!val) {
+   if (!xenbus_read_unsigned(dev->otherend, "feature-rx-notify", 0)) {
/* - Reduce drain timeout to poll more frequently for
 *   Rx requests.
 * - Disable Rx stall detection.
@@ -1162,34 +1153,21 @@ static int read_xenbus_vif_flags(struct backend_info 
*be)
be->vif->stall_timeout = 0;
}
 
-   if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-sg",
-"%d", ) < 0)
-   val = 0;
-   vif->can_sg = !!val;
+   vif->can_sg = !!xenbus_read_unsigned(dev->otherend, "feature-sg", 0);
 
vif->gso_mask = 0;
 
-   if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4",
-"%d", ) < 0)
-   val = 0;
-   if (val)
+   if (xenbus_read_unsigned(dev->otherend, "feature-gso-tcpv4", 0))
vif->gso_mask |= GSO_BIT(TCPV4);
 
-   if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv6",
-"%d", ) < 0)
-   val = 0;
-   if (val)
+   if (xenbus_read_unsigned(dev->otherend, "feature-gso-tcpv6", 0))
vif->gso_mask |= GSO_BIT(TCPV6);
 
-   if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-no-csum-offload",
-"%d", ) < 0)
-   val = 0;
-   vif->ip_csum = !val;
+   vif->ip_csum = !xenbus_read_unsigned(dev->otherend,
+"feature-no-csum-offload", 0);
 
-   if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-ipv6-csum-offload",
-"%d", ) < 0)
-   val = 0;
-   vif->ipv6_csum = !!val;
+   vif->ipv6_csum = !!xenbus_read_unsigned(dev->otherend,
+   "feature-ipv6-csum-offload", 0);
 
return 0;
 }
-- 
2.6.6

Re: [Xen-devel] [PATCH resend] xen-netback: switch to threaded irq for control ring

[Juergen Gross]

On 22/09/16 12:31, Paul Durrant wrote:
>> -Original Message-
>> From: Juergen Gross [mailto:jgr...@suse.com]
>> Sent: 22 September 2016 11:17
>> To: Paul Durrant <paul.durr...@citrix.com>; xen-de...@lists.xenproject.org;
>> net...@vger.kernel.orga <netdev@vger.kernel.org>; linux-
>> ker...@vger.kernel.org
>> Cc: Wei Liu <wei.l...@citrix.com>
>> Subject: Re: [Xen-devel] [PATCH resend] xen-netback: switch to threaded irq
>> for control ring
>>
>> On 22/09/16 11:09, Paul Durrant wrote:
>>>> -Original Message-----
>>>> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of
>>>> Juergen Gross
>>>> Sent: 22 September 2016 10:03
>>>> To: xen-de...@lists.xenproject.org; net...@vger.kernel.orga; linux-
>>>> ker...@vger.kernel.org
>>>> Cc: Juergen Gross <jgr...@suse.com>; Wei Liu <wei.l...@citrix.com>
>>>> Subject: [Xen-devel] [PATCH resend] xen-netback: switch to threaded
>>>> irq for control ring
>>>>
>>>> Instead of open coding it use the threaded irq mechanism in xen-netback.
>>>>
>>>> Signed-off-by: Juergen Gross <jgr...@suse.com>
>>>
>>> How have you tested this change?
>>
>> Only compile-tested and loaded the module. As this feature isn't being used
>> in linux netfront AFAIK it is not easily testable. OTOH the code 
>> modification is
>> rather limited and I've used the threaded irq in the Xen scsiback driver
>> myself, so I'm rather confident it will work.
>>
> 
> OK. How about doing the rx interrupt/task too so that it can be easily tested 
> with a linux netfront?

I'd like to, but this would require some more work. The rx kthread isn't
activated by an event only, but by a timer, too. This isn't easy to map
to the threaded irq framework. If, however, you are confident the timer
isn't really necessary I'd be happy to provide a patch switching the
rx task to the threaded irq, too.

And to be honest: this wouldn't verify that the control ring related
patch is really working. The mechanism itself _is_ working as it is
already in use in xen-scsiback in a very similar environment.


Juergen

Re: [Xen-devel] [PATCH resend] xen-netback: switch to threaded irq for control ring

[Juergen Gross]

On 22/09/16 11:09, Paul Durrant wrote:
>> -Original Message-
>> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of
>> Juergen Gross
>> Sent: 22 September 2016 10:03
>> To: xen-de...@lists.xenproject.org; net...@vger.kernel.orga; linux-
>> ker...@vger.kernel.org
>> Cc: Juergen Gross <jgr...@suse.com>; Wei Liu <wei.l...@citrix.com>
>> Subject: [Xen-devel] [PATCH resend] xen-netback: switch to threaded irq for
>> control ring
>>
>> Instead of open coding it use the threaded irq mechanism in xen-netback.
>>
>> Signed-off-by: Juergen Gross <jgr...@suse.com>
> 
> How have you tested this change?

Only compile-tested and loaded the module. As this feature isn't being
used in linux netfront AFAIK it is not easily testable. OTOH the code
modification is rather limited and I've used the threaded irq in the
Xen scsiback driver myself, so I'm rather confident it will work.


Juergen

[PATCH resend 2] xen-netback: switch to threaded irq for control ring

[Juergen Gross]

Instead of open coding it use the threaded irq mechanism in
xen-netback.

Signed-off-by: Juergen Gross <jgr...@suse.com>
---
resend due to missing netdev list in first attempt and wrong address in second.
---
 drivers/net/xen-netback/common.h|  4 +---
 drivers/net/xen-netback/interface.c | 38 ++---
 drivers/net/xen-netback/netback.c   | 18 --
 3 files changed, 11 insertions(+), 49 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 84d6cbd..b38fb2c 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -292,8 +292,6 @@ struct xenvif {
 #endif
 
struct xen_netif_ctrl_back_ring ctrl;
-   struct task_struct *ctrl_task;
-   wait_queue_head_t ctrl_wq;
unsigned int ctrl_irq;
 
/* Miscellaneous private stuff. */
@@ -359,7 +357,7 @@ void xenvif_kick_thread(struct xenvif_queue *queue);
 
 int xenvif_dealloc_kthread(void *data);
 
-int xenvif_ctrl_kthread(void *data);
+irqreturn_t xenvif_ctrl_irq_fn(int irq, void *data);
 
 void xenvif_rx_queue_tail(struct xenvif_queue *queue, struct sk_buff *skb);
 
diff --git a/drivers/net/xen-netback/interface.c 
b/drivers/net/xen-netback/interface.c
index 83deeeb..fb50c6d 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -128,15 +128,6 @@ irqreturn_t xenvif_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
 }
 
-irqreturn_t xenvif_ctrl_interrupt(int irq, void *dev_id)
-{
-   struct xenvif *vif = dev_id;
-
-   wake_up(>ctrl_wq);
-
-   return IRQ_HANDLED;
-}
-
 int xenvif_queue_stopped(struct xenvif_queue *queue)
 {
struct net_device *dev = queue->vif->dev;
@@ -570,8 +561,7 @@ int xenvif_connect_ctrl(struct xenvif *vif, grant_ref_t 
ring_ref,
struct net_device *dev = vif->dev;
void *addr;
struct xen_netif_ctrl_sring *shared;
-   struct task_struct *task;
-   int err = -ENOMEM;
+   int err;
 
err = xenbus_map_ring_valloc(xenvif_to_xenbus_device(vif),
 _ref, 1, );
@@ -581,11 +571,7 @@ int xenvif_connect_ctrl(struct xenvif *vif, grant_ref_t 
ring_ref,
shared = (struct xen_netif_ctrl_sring *)addr;
BACK_RING_INIT(>ctrl, shared, XEN_PAGE_SIZE);
 
-   init_waitqueue_head(>ctrl_wq);
-
-   err = bind_interdomain_evtchn_to_irqhandler(vif->domid, evtchn,
-   xenvif_ctrl_interrupt,
-   0, dev->name, vif);
+   err = bind_interdomain_evtchn_to_irq(vif->domid, evtchn);
if (err < 0)
goto err_unmap;
 
@@ -593,19 +579,13 @@ int xenvif_connect_ctrl(struct xenvif *vif, grant_ref_t 
ring_ref,
 
xenvif_init_hash(vif);
 
-   task = kthread_create(xenvif_ctrl_kthread, (void *)vif,
- "%s-control", dev->name);
-   if (IS_ERR(task)) {
-   pr_warn("Could not allocate kthread for %s\n", dev->name);
-   err = PTR_ERR(task);
+   err = request_threaded_irq(vif->ctrl_irq, NULL, xenvif_ctrl_irq_fn,
+  IRQF_ONESHOT, "xen-netback-ctrl", vif);
+   if (err) {
+   pr_warn("Could not setup irq handler for %s\n", dev->name);
goto err_deinit;
}
 
-   get_task_struct(task);
-   vif->ctrl_task = task;
-
-   wake_up_process(vif->ctrl_task);
-
return 0;
 
 err_deinit:
@@ -774,12 +754,6 @@ void xenvif_disconnect_data(struct xenvif *vif)
 
 void xenvif_disconnect_ctrl(struct xenvif *vif)
 {
-   if (vif->ctrl_task) {
-   kthread_stop(vif->ctrl_task);
-   put_task_struct(vif->ctrl_task);
-   vif->ctrl_task = NULL;
-   }
-
if (vif->ctrl_irq) {
xenvif_deinit_hash(vif);
unbind_from_irqhandler(vif->ctrl_irq, vif);
diff --git a/drivers/net/xen-netback/netback.c 
b/drivers/net/xen-netback/netback.c
index edbae0b..3d0c989 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -2359,24 +2359,14 @@ static bool xenvif_ctrl_work_todo(struct xenvif *vif)
return 0;
 }
 
-int xenvif_ctrl_kthread(void *data)
+irqreturn_t xenvif_ctrl_irq_fn(int irq, void *data)
 {
struct xenvif *vif = data;
 
-   for (;;) {
-   wait_event_interruptible(vif->ctrl_wq,
-xenvif_ctrl_work_todo(vif) ||
-kthread_should_stop());
-   if (kthread_should_stop())
-   break;
+   while (xenvif_ctrl_work_todo(vif))
+   xenvif_ctrl_action(vif);
 
-   while (xenvif_ctrl_work_todo(vif))
-   xenvif_ctrl_action(vif);
-
-