Re: [Xen-devel] [PATCH 4/6] xen-netfront: add range check for Tx response id

[Wei Liu]

On Mon, Apr 30, 2018 at 11:01:48PM +0200, Marek Marczykowski-Górecki wrote:
> Tx response ID is fetched from shared page, so make sure it is sane
> before using it as an array index.
> 
> CC: sta...@vger.kernel.org
> Signed-off-by: Marek Marczykowski-Górecki 
> ---
>  drivers/net/xen-netfront.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index 934b8a4..55c9b25 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -394,6 +394,7 @@ static void xennet_tx_buf_gc(struct netfront_queue *queue)
>   continue;
>  
>   id  = txrsp.id;
> + BUG_ON(id >= NET_TX_RING_SIZE);

It is better to use ARRAY_SIZE here.

Wei.

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

[Wei Liu]

On Tue, Apr 24, 2018 at 03:18:14PM +0200, 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 <luc.vanoostenr...@gmail.com>

Reviewed-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH] xen-netback: fix xenvif_start_xmit()'s return type

[Wei Liu]

On Tue, Apr 24, 2018 at 03:18:12PM +0200, 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 <luc.vanoostenr...@gmail.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

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

[Wei Liu]

On Fri, Apr 20, 2018 at 10:47:31AM +, Stanislav Kinsburskii wrote:
>  
>  #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();

If you care about the return value, please propagate it to
netback_init's caller. Otherwise you can just make the function return
void.

> +
> +int xenvif_fi_init(struct xenvif *vif)
> +{
> + struct dentry *parent;
> + struct xenvif_fi *vfi;
> + int fi, err = -ENOMEM;
> +
> + parent = vif_fi_dir;
> + if (!parent)
> + return -ENOMEM;
> +
> + vfi = kmalloc(sizeof(*vfi), GFP_KERNEL);
> + if (!vfi)
> + return -ENOMEM;
> +
> + vfi->dir = debugfs_create_dir(vif->dev->name, parent);
> + if (!vfi->dir)
> + goto err_dir;
> +
> + for (fi = 0; fi < XENVIF_FI_MAX; fi++) {
> + vfi->faults[fi] = xen_fi_dir_add(vfi->dir,
> + xenvif_fi_names[fi]);
> + if (!vfi->faults[fi])
> + goto err_fault;
> + }
> +
> + vif->fi_info = vfi;
> + return 0;
> +
> +err_fault:
> + for (; fi > 0; fi--)

fi >= 0

Wei.

[RFC PATCH 1/3] qemu: virtio-bypass should explicitly bind to a passthrough device

[Si-Wei Liu]

The new backup option allows guest virtio-bypass driver to explicitly
bind to a corresponding passthrough instance, which is identifiable by
the :. notation. MAC address is still validated
in the guest but not the only criteria for pairing two devices.
MAC address is more a matter of network configuration than a (virtual)
device identifier, the latter of which needs to be unique as part of
VM configuration. Techinically it's possible there exists more than
one device in the guest configured with the same MAC, but each belongs
to completely isolated network.

The direct benefit as a result of the explicit binding (or pairing),
apparently, is the prohibition of improper binding or malicious pairing
due to any flexiblility in terms of guest MAC address config.

What's more important, the indicator of guest device location can even
be used as a means to reserve the slot for the corresponding passthrough
device in the PCI bus tree if such device is temporarily absent, but
yet to be hot plugged into the VM. We'd need to preserve the slot for
the passthrough device to which virtio-bypass is bound, such that once
it is plugged out as a result of migration we can ensure the slot
wouldn't be occupied by other devices, and any user-space application
assumes consistent device location in the bus tree still works.

The usage for the backup option is as follows:

   -device virtio-net-pci, ... ,backup=:[.]

for e.g.

   -device virtio-net-pci,id=net0,mac=52:54:00:e0:58:80,backup=pci.2:0x3
   ...
   -device vfio-pci,host=02:10.1,id=hostdev0,bus=pci.2,addr=0x3

Signed-off-by: Si-Wei Liu <si-wei@oracle.com>
---
 hw/net/virtio-net.c | 29 -
 include/hw/pci/pci.h|  3 ++
 include/hw/virtio/virtio-net.h  |  2 +
 include/standard-headers/linux/virtio_net.h |  1 +
 qdev-monitor.c  | 64 +
 5 files changed, 97 insertions(+), 2 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index de31b1b98c..a36b169958 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -26,6 +26,7 @@
 #include "qapi-event.h"
 #include "hw/virtio/virtio-access.h"
 #include "migration/misc.h"
+#include "hw/pci/pci.h"
 
 #define VIRTIO_NET_VM_VERSION11
 
@@ -61,6 +62,8 @@ static VirtIOFeature feature_sizes[] = {
  .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
 {.flags = 1 << VIRTIO_NET_F_MTU,
  .end = endof(struct virtio_net_config, mtu)},
+{.flags = 1 << VIRTIO_NET_F_BACKUP,
+ .end = endof(struct virtio_net_config, bsf2backup)},
 {}
 };
 
@@ -84,10 +87,24 @@ static void virtio_net_get_config(VirtIODevice *vdev, 
uint8_t *config)
 {
 VirtIONet *n = VIRTIO_NET(vdev);
 struct virtio_net_config netcfg;
+uint16_t busdevfn;
 
 virtio_stw_p(vdev, , n->status);
 virtio_stw_p(vdev, _virtqueue_pairs, n->max_queues);
 virtio_stw_p(vdev, , n->net_conf.mtu);
+if (n->net_conf.backup) {
+/* Below function should not fail as the backup ID string
+ * has been validated when device is being realized.
+ * Until guest starts to run we can can get to the
+ * effective bus num in use from pci config space where
+ * guest had written to.
+ */
+pci_get_busdevfn_by_id(n->net_conf.backup, ,
+   NULL, NULL);
+busdevfn <<= 8;
+busdevfn |= (n->backup_devfn & 0xFF);
+virtio_stw_p(vdev, , busdevfn);
+}
 memcpy(netcfg.mac, n->mac, ETH_ALEN);
 memcpy(config, , n->config_size);
 }
@@ -1935,11 +1952,19 @@ static void virtio_net_device_realize(DeviceState *dev, 
Error **errp)
 VirtIODevice *vdev = VIRTIO_DEVICE(dev);
 VirtIONet *n = VIRTIO_NET(dev);
 NetClientState *nc;
+uint16_t bdevfn;
 int i;
 
 if (n->net_conf.mtu) {
 n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
 }
+if (n->net_conf.backup) {
+if (pci_get_busdevfn_by_id(n->net_conf.backup, NULL,
+   , errp))
+return;
+n->backup_devfn = bdevfn;
+n->host_features |= (0x1 << VIRTIO_NET_F_BACKUP);
+}
 
 virtio_net_set_config_size(n, n->host_features);
 virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
@@ -2160,8 +2185,8 @@ static Property virtio_net_properties[] = {
 DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
 DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, mtu_bypass_backend,
  true),
-DEFINE_PROP_BIT("backup", VirtIONet, host_features,
- VIRTIO_NET_F_BACKUP, false),
+DEFINE_PROP_STRING("backup", VirtIONet, net_conf.backup),
+
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/pci/pci.h b/include/hw/p

[RFC PATCH 0/3] Userspace compatible driver model for virtio_bypass

[Si-Wei Liu]

This RFC patch series attempts to hide the lower netdevs for virtio_bypass
from userspace visibility, and tighten up the association between virtio_bypass
and the lower passthrough netdev to be enslaved by binding to a specific device
identifier explicitly. This in turn has the benefits of taking the merit of the
2-netdev driver model from netvsc (userspace compliance) to a perfect sense,
while keeping the internal implementation still a 3-netdev model. There's no
loss of feature such as XDP, and continously adding improvements for performance
and features thanks to the good bypass nature of the 3-netdev model are also
possible in the long run.

As said, this change should make the code sharing between netvsc and 
virtio_bypass
easier and more approachable, as I think the concerns Stephen pointed out was
mainly regarding userspace compatibility and not the hardware offloading
tunables on the VF slave that had to be exposed to netvsc users today, if I'm
not mistaken.

Jiri expressed concerns around the weak check depending on MAC address only
during enslavement and we really need to do strict checks more than that. With
the change to requiring user explicitly specifying the passthrough device
to which virtio_bypass is expected to be bound, virtio_bypass now would match
device based on the PCI slot info in device tree, rather than rely on MAC
address inadvertently. In addition, the PCI slot info passed in will be helpful
to accommodate udevd to name the virtio_bypass interface specifically, making
a transparent and automatic upgrade from existing VF setup to virtio_bypass
possible (expect udevd patch to come later on).

Since I'd like to get the discussion going as early as possible, this series
just shows essential changes to a minimal set. Although not included in the
series, I would like to remind ahead that a few neccessary pieces must be built
upon the assumption of hidden lower netdevs and explicit binding. Such as
sysfs interfaces for udev's naming of virtio_bypass interace. Such as passing
down HW offloading configs to the active lower slave, and making it persistent
across live migration. And so on..

The current patch series is based on Sridhar's v4 patch "Enable virtio to act
as a backup for a passthru device", but I can resync anyway to his upcoming
version once posted.


Si-Wei Liu (1):
  qemu: virtio-bypass should explicitly bind to a passthrough device

 hw/net/virtio-net.c | 29 -
 include/hw/pci/pci.h|  3 ++
 include/hw/virtio/virtio-net.h  |  2 +
 include/standard-headers/linux/virtio_net.h |  1 +
 qdev-monitor.c  | 64 +
 5 files changed, 97 insertions(+), 2 deletions(-)

Si-Wei Liu (2):
  netdev: kernel-only IFF_HIDDEN netdevice
  virtio_net: make lower netdevs for virtio_bypass hidden

 drivers/net/virtio_net.c| 159 +--
 include/linux/netdevice.h   |  12 ++
 include/net/net_namespace.h |   2 +
 include/uapi/linux/virtio_net.h |   2 +
 net/core/dev.c  | 281 +++-
 net/core/net_namespace.c|   1 +
 6 files changed, 411 insertions(+), 46 deletions(-)

-- 
1.8.3.1

[RFC PATCH 3/3] virtio_net: make lower netdevs for virtio_bypass hidden

[Si-Wei Liu]

We should move virtio_bypass to a 1-upper-with-2-hidden-lower
driver model for greater compatibility with regard to preserving
userpsace API and ABI.

On the other hand, technically virtio_bypass should make stricter
check before automatically enslaving the corresponding virtual
function or passthrough device. It's more reasonable to pair
virtio_bypass instance with a VF or passthrough device 1:1,
rather than rely on searching for a random non-virtio netdev with
exact same MAC address. One possible way of doing it is to bind
virtio_bypass explicitly to a guest pci device by specifying
its  and : location. Changing BACKUP feature
to take these configs into account, such that verifying target
device for auto-enslavement no longer relies on the MAC address.

Signed-off-by: Si-Wei Liu <si-wei@oracle.com>
---
 drivers/net/virtio_net.c| 159 
 include/uapi/linux/virtio_net.h |   2 +
 2 files changed, 148 insertions(+), 13 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f850cf6..c54a5bd 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -77,6 +77,8 @@ struct virtnet_stats {
u64 rx_packets;
 };
 
+static struct workqueue_struct *virtnet_bypass_wq;
+
 /* Internal representation of a send virtqueue */
 struct send_queue {
/* Virtqueue associated with this send _queue */
@@ -196,6 +198,13 @@ struct padded_vnet_hdr {
char padding[4];
 };
 
+struct virtnet_bypass_task {
+   struct work_struct  work;
+   unsigned long   event; 
+   struct net_device   *child_netdev;
+   struct net_device   *bypass_netdev;
+};
+
 /* Converting between virtqueue no. and kernel tx/rx queue no.
  * 0:rx0 1:tx0 2:rx1 3:tx1 ... 2N:rxN 2N+1:txN 2N+2:cvq
  */
@@ -2557,6 +2566,11 @@ struct virtnet_bypass_info {
 
/* spinlock while updating stats */
spinlock_t stats_lock;
+
+   int bus;
+   int slot;
+   int function;
+
 };
 
 static void virtnet_bypass_child_open(struct net_device *dev,
@@ -2822,10 +2836,13 @@ static void virtnet_bypass_ethtool_get_drvinfo(struct 
net_device *dev,
.get_link_ksettings = virtnet_bypass_ethtool_get_link_ksettings,
 };
 
-static struct net_device *get_virtnet_bypass_bymac(struct net *net,
-  const u8 *mac)
+static struct net_device *
+get_virtnet_bypass_bymac(struct net_device *child_netdev)
 {
+   struct net *net = dev_net(child_netdev);
struct net_device *dev;
+   struct virtnet_bypass_info *vbi;
+   int devfn;
 
ASSERT_RTNL();
 
@@ -2833,7 +2850,29 @@ static struct net_device 
*get_virtnet_bypass_bymac(struct net *net,
if (dev->netdev_ops != _bypass_netdev_ops)
continue;   /* not a virtnet_bypass device */
 
-   if (ether_addr_equal(mac, dev->perm_addr))
+   if (!ether_addr_equal(child_netdev->dev_addr, dev->perm_addr))
+   continue;   /* not matching MAC address */
+
+   if (!child_netdev->dev.parent)
+   continue;
+
+   /* Is child_netdev a backup netdev ? */
+   if (child_netdev->dev.parent == dev->dev.parent)
+   return dev;
+
+   /* Avoid non pci devices as active netdev */
+   if (!dev_is_pci(child_netdev->dev.parent))
+   continue;
+
+   vbi = netdev_priv(dev);
+   devfn = PCI_DEVFN(vbi->slot, vbi->function);
+
+   netdev_info(dev, "bus %d slot %d func %d",
+   vbi->bus, vbi->slot, vbi->function);
+
+   /* Need to match :. */
+   if (pci_get_bus_and_slot(vbi->bus, devfn) ==
+   to_pci_dev(child_netdev->dev.parent))
return dev;
}
 
@@ -2878,10 +2917,61 @@ static rx_handler_result_t 
virtnet_bypass_handle_frame(struct sk_buff **pskb)
return RX_HANDLER_ANOTHER;
 }
 
+static int virtnet_bypass_pregetname_child(struct net_device *child_netdev)
+{
+   struct net_device *dev;
+
+   if (child_netdev->addr_len != ETH_ALEN)
+   return NOTIFY_DONE;
+
+   /* We will use the MAC address to locate the virtnet_bypass netdev
+* to associate with the child netdev. If we don't find a matching
+* bypass netdev, move on.
+*/
+   dev = get_virtnet_bypass_bymac(child_netdev);
+   if (!dev)
+   return NOTIFY_DONE;
+
+   if (child_netdev->dev.parent &&
+   child_netdev->dev.parent != dev->dev.parent);
+   netdev_set_hidden(child_netdev);
+
+   return NOTIFY_OK;
+}
+
+static void virtnet_bypass_task_fn(struct work_struct *work)
+{
+   struct virtnet_bypass_task *task;
+   struct net_device *child_netdev;
+  

[RFC PATCH 2/3] netdev: kernel-only IFF_HIDDEN netdevice

[Si-Wei Liu]

Hidden netdevice is not visible to userspace such that
typical network utilites e.g. ip, ifconfig and et al,
cannot sense its existence or configure it. Internally
hidden netdev may associate with an upper level netdev
that userspace has access to. Although userspace cannot
manipulate the lower netdev directly, user may control
or configure the underlying hidden device through the
upper-level netdev. For identification purpose, the
kobject for hidden netdev still presents in the sysfs
hierarchy, however, no uevent message will be generated
when the sysfs entry is created, modified or destroyed.

For that end, a separate namescope needs to be carved
out for IFF_HIDDEN netdevs. As of now netdev name that
starts with colon i.e. ':' is invalid in userspace,
since socket ioctls such as SIOCGIFCONF use ':' as the
separator for ifname. The absence of namescope started
with ':' can rightly be used as the namescope for
the kernel-only IFF_HIDDEN netdevs.

Signed-off-by: Si-Wei Liu <si-wei@oracle.com>
---
 include/linux/netdevice.h   |  12 ++
 include/net/net_namespace.h |   2 +
 net/core/dev.c  | 281 ++--
 net/core/net_namespace.c|   1 +
 4 files changed, 263 insertions(+), 33 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ef789e1..1a70f3a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1380,6 +1380,7 @@ struct net_device_ops {
  * @IFF_PHONY_HEADROOM: the headroom value is controlled by an external
  * entity (i.e. the master device for bridged veth)
  * @IFF_MACSEC: device is a MACsec device
+ * @IFF_HIDDEN: device is not visible to userspace
  */
 enum netdev_priv_flags {
IFF_802_1Q_VLAN = 1<<0,
@@ -1410,6 +1411,7 @@ enum netdev_priv_flags {
IFF_RXFH_CONFIGURED = 1<<25,
IFF_PHONY_HEADROOM  = 1<<26,
IFF_MACSEC  = 1<<27,
+   IFF_HIDDEN  = 1<<28,
 };
 
 #define IFF_802_1Q_VLANIFF_802_1Q_VLAN
@@ -1439,6 +1441,7 @@ enum netdev_priv_flags {
 #define IFF_TEAM   IFF_TEAM
 #define IFF_RXFH_CONFIGUREDIFF_RXFH_CONFIGURED
 #define IFF_MACSEC IFF_MACSEC
+#define IFF_HIDDEN IFF_HIDDEN
 
 /**
  * struct net_device - The DEVICE structure.
@@ -1659,6 +1662,7 @@ enum netdev_priv_flags {
 struct net_device {
charname[IFNAMSIZ];
struct hlist_node   name_hlist;
+   struct hlist_node   name_cmpl_hlist;
struct dev_ifalias  __rcu *ifalias;
/*
 *  I/O specific fields
@@ -1680,6 +1684,7 @@ struct net_device {
unsigned long   state;
 
struct list_headdev_list;
+   struct list_headdev_cmpl_list;
struct list_headnapi_list;
struct list_headunreg_list;
struct list_headclose_list;
@@ -2326,6 +2331,7 @@ struct netdev_lag_lower_state_info {
 #define NETDEV_UDP_TUNNEL_PUSH_INFO0x001C
 #define NETDEV_UDP_TUNNEL_DROP_INFO0x001D
 #define NETDEV_CHANGE_TX_QUEUE_LEN 0x001E
+#define NETDEV_PRE_GETNAME 0x001F
 
 int register_netdevice_notifier(struct notifier_block *nb);
 int unregister_netdevice_notifier(struct notifier_block *nb);
@@ -2393,6 +2399,8 @@ static inline void netdev_notifier_info_init(struct 
netdev_notifier_info *info,
for_each_netdev_rcu(_net, slave)   \
if (netdev_master_upper_dev_get_rcu(slave) == (bond))
 #define net_device_entry(lh)   list_entry(lh, struct net_device, dev_list)
+#define for_each_netdev_complete(net, d)   \
+   list_for_each_entry(d, &(net)->dev_cmpl_head, dev_cmpl_list)
 
 static inline struct net_device *next_net_device(struct net_device *dev)
 {
@@ -2462,6 +2470,10 @@ static inline void unregister_netdevice(struct 
net_device *dev)
unregister_netdevice_queue(dev, NULL);
 }
 
+void netdev_set_hidden(struct net_device *dev);
+int hide_netdevice(struct net_device *dev);
+void unhide_netdevice(struct net_device *dev);
+
 int netdev_refcnt_read(const struct net_device *dev);
 void free_netdev(struct net_device *dev);
 void netdev_freemem(struct net_device *dev);
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 0490084..f9ce9b4 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -80,7 +80,9 @@ struct net {
struct sock *genl_sock;
 
struct list_headdev_base_head;
+   struct list_headdev_cmpl_head;
struct hlist_head   *dev_name_head;
+   struct hlist_head   *dev_name_cmpl_head;
struct hlist_head   *dev_index_head;
unsigned intdev_base_seq;   /* protected by rtnl_mutex */
int ifindex;
diff --git a/ne

Re: [PATCH 4/4] drivers/net: Use octal not symbolic permissions

[Wei Liu]

On Fri, Mar 23, 2018 at 03:54:39PM -0700, Joe Perches wrote:
> Prefer the direct use of octal for permissions.
> 
> Done with checkpatch -f --types=SYMBOLIC_PERMS --fix-inplace
> and some typing.
> 
> Miscellanea:
> 
> o Whitespace neatening around these conversions.
> 
> Signed-off-by: Joe Perches <j...@perches.com>
> ---
>  drivers/net/xen-netback/xenbus.c   |  4 +-
>  drivers/net/xen-netfront.c |  6 +--

Reviewed-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH][next] xen-netback: make function xenvif_rx_skb static

[Wei Liu]

On Fri, Feb 23, 2018 at 05:16:57PM +, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> The function xenvif_rx_skb is local to the source and does not need
> to be in global scope, so make it static.
> 
> Cleans up sparse warning:
> drivers/net/xen-netback/rx.c:422:6: warning: symbol 'xenvif_rx_skb'
> was not declared. Should it be static?
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Thanks

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

[Wei Liu]

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();
> + }
> +
>   xennet_disconnect_backend(info);
>  
>   unregister_netdev(info->netdev);
> -- 
> 2.13.6
> 

Re: [PATCH 54/58] net/xen-netback: Convert timers to use timer_setup()

[Wei Liu]

On Mon, Oct 16, 2017 at 05:29:38PM -0700, Kees Cook wrote:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
> 
> Cc: Wei Liu <wei.l...@citrix.com>
> Cc: Paul Durrant <paul.durr...@citrix.com>
> Cc: xen-de...@lists.xenproject.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keesc...@chromium.org>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH] xen-netfront, xen-netback: Use correct minimum MTU values

[Wei Liu]

On Mon, Oct 16, 2017 at 03:20:32PM +0200, Mohammed Gamal wrote:
> RFC791 specifies the minimum MTU to be 68, while xen-net{front|back}
> drivers use a minimum value of 0.
> 
> When set MTU to 0~67 with xen_net{front|back} driver, the network
> will become unreachable immediately, the guest can no longer be pinged.
> 
> xen_net{front|back} should not allow the user to set this value which causes
> network problems.
> 
> Reported-by: Chen Shi <che...@redhat.com>
> Signed-off-by: Mohammed Gamal <mga...@redhat.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

CC netfront maintainers

Re: [PATCH net-next] xen-netback: update ubuf_info initialization to anonymous union

[Wei Liu]

On Fri, Aug 25, 2017 at 01:10:43PM -0400, Willem de Bruijn wrote:
> From: Willem de Bruijn <will...@google.com>
> 
> The xen driver initializes struct ubuf_info fields using designated
> initializers. I recently moved these fields inside a nested anonymous
> struct inside an anonymous union. I had missed this use case.
> 
> This breaks compilation of xen-netback with older compilers.
> From kbuild bot with gcc-4.4.7:
> 
>drivers/net//xen-netback/interface.c: In function
>'xenvif_init_queue':
>>> drivers/net//xen-netback/interface.c:554: error: unknown field 'ctx' 
> specified in initializer
>>> drivers/net//xen-netback/interface.c:554: warning: missing braces 
> around initializer
>   drivers/net//xen-netback/interface.c:554: warning: (near initialization 
> for '(anonymous).')
>>> drivers/net//xen-netback/interface.c:554: warning: initialization makes 
> integer from pointer without a cast
>>> drivers/net//xen-netback/interface.c:555: error: unknown field 'desc' 
> specified in initializer
> 
> Add double braces around the designated initializers to match their
> nested position in the struct. After this, compilation succeeds again.
> 
> Fixes: 4ab6c99d99bb ("sock: MSG_ZEROCOPY notification coalescing")
> Reported-by: kbuild bot <l...@intel.com>
> Signed-off-by: Willem de Bruijn <will...@google.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

[PATCH net] xen-netback: correctly schedule rate-limited queues

[Wei Liu]

Add a flag to indicate if a queue is rate-limited. Test the flag in
NAPI poll handler and avoid rescheduling the queue if true, otherwise
we risk locking up the host. The rescheduling will be done in the
timer callback function.

Reported-by: Jean-Louis Dupond <jean-lo...@dupond.be>
Signed-off-by: Wei Liu <wei.l...@citrix.com>
Tested-by: Jean-Louis Dupond <jean-lo...@dupond.be>
---
 drivers/net/xen-netback/common.h| 1 +
 drivers/net/xen-netback/interface.c | 6 +-
 drivers/net/xen-netback/netback.c   | 6 +-
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 530586be05b4..5b1d2e8402d9 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -199,6 +199,7 @@ struct xenvif_queue { /* Per-queue data for xenvif */
unsigned long   remaining_credit;
struct timer_list credit_timeout;
u64 credit_window_start;
+   bool rate_limited;
 
/* Statistics */
struct xenvif_stats stats;
diff --git a/drivers/net/xen-netback/interface.c 
b/drivers/net/xen-netback/interface.c
index 8397f6c92451..e322a862ddfe 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -106,7 +106,11 @@ static int xenvif_poll(struct napi_struct *napi, int 
budget)
 
if (work_done < budget) {
napi_complete_done(napi, work_done);
-   xenvif_napi_schedule_or_enable_events(queue);
+   /* If the queue is rate-limited, it shall be
+* rescheduled in the timer callback.
+*/
+   if (likely(!queue->rate_limited))
+   xenvif_napi_schedule_or_enable_events(queue);
}
 
return work_done;
diff --git a/drivers/net/xen-netback/netback.c 
b/drivers/net/xen-netback/netback.c
index 602d408fa25e..5042ff8d449a 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -180,6 +180,7 @@ static void tx_add_credit(struct xenvif_queue *queue)
max_credit = ULONG_MAX; /* wrapped: clamp to ULONG_MAX */
 
queue->remaining_credit = min(max_credit, max_burst);
+   queue->rate_limited = false;
 }
 
 void xenvif_tx_credit_callback(unsigned long data)
@@ -686,8 +687,10 @@ static bool tx_credit_exceeded(struct xenvif_queue *queue, 
unsigned size)
msecs_to_jiffies(queue->credit_usec / 1000);
 
/* Timer could already be pending in rare cases. */
-   if (timer_pending(>credit_timeout))
+   if (timer_pending(>credit_timeout)) {
+   queue->rate_limited = true;
return true;
+   }
 
/* Passed the point where we can replenish credit? */
if (time_after_eq64(now, next_credit)) {
@@ -702,6 +705,7 @@ static bool tx_credit_exceeded(struct xenvif_queue *queue, 
unsigned size)
mod_timer(>credit_timeout,
  next_credit);
queue->credit_window_start = next_credit;
+   queue->rate_limited = true;
 
return true;
}
-- 
2.11.0

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

[Wei Liu]

On Thu, Mar 02, 2017 at 12:54:26PM +, 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>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH net 1/2] xen-netback: keep a local pointer for vif in backend_disconnect()

[Wei Liu]

On Thu, Mar 02, 2017 at 12:54:25PM +, Paul Durrant wrote:
> This patch replaces use of 'be->vif' with 'vif' and hence generally
> makes the function look tidier. No semantic change.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

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

[Wei Liu]

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?

> 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: [PATCH net] xen-netback: Use GFP_ATOMIC to allocate hash

[Wei Liu]

On Thu, Mar 02, 2017 at 10:50:20AM +, Anoob Soman wrote:
> Allocation of new_hash, inside xenvif_new_hash(), always happen
> in softirq context, so use GFP_ATOMIC instead of GFP_KERNEL for new
> hash allocation.
> 
> Signed-off-by: Anoob Soman <anoob.so...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH v2 2/2] xen-netback: protect resource cleaning on XenBus disconnect

[Wei Liu]

On Tue, Jan 17, 2017 at 08:49:38PM +, Igor Druzhinin wrote:
> vif->lock is used to protect statistics gathering agents from using the
> queue structure during cleaning.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH v2 1/2] xen-netback: fix memory leaks on XenBus disconnect

[Wei Liu]

On Tue, Jan 17, 2017 at 08:49:37PM +, Igor Druzhinin wrote:
> Eliminate memory leaks introduced several years ago by cleaning the
> queue resources which are allocated on XenBus connection event. Namely, queue
> structure array and pages used for IO rings.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhi...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH] xen-netback: fix memory leaks on XenBus disconnect

[Wei Liu]

On Thu, Jan 12, 2017 at 05:51:56PM +, Igor Druzhinin wrote:
> Eliminate memory leaks introduced several years ago by cleaning the queue
> resources which are allocated on XenBus connection event. Namely, queue
> structure array and pages used for IO rings.
> vif->lock is used to protect statistics gathering agents from using the
> queue structure during cleaning.
> 

There is code in netback_remove which eventually calls xenvif_free to
free up the resources, maybe you should modify xenvif_free instead? That
seems more symmetric to me. What do you think?

> Signed-off-by: Igor Druzhinin 
> ---
>  drivers/net/xen-netback/interface.c |  6 --
>  drivers/net/xen-netback/xenbus.c| 13 +
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/interface.c 
> b/drivers/net/xen-netback/interface.c
> index e30ffd2..5795213 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -221,18 +221,18 @@ static struct net_device_stats *xenvif_get_stats(struct 
> net_device *dev)
>  {
>   struct xenvif *vif = netdev_priv(dev);
>   struct xenvif_queue *queue = NULL;
> - unsigned int num_queues = vif->num_queues;
>   unsigned long rx_bytes = 0;
>   unsigned long rx_packets = 0;
>   unsigned long tx_bytes = 0;
>   unsigned long tx_packets = 0;
>   unsigned int index;
>  
> + spin_lock(>lock);
>   if (vif->queues == NULL)
>   goto out;
>  
>   /* Aggregate tx and rx stats from each queue */
> - for (index = 0; index < num_queues; ++index) {
> + for (index = 0; index < vif->num_queues; ++index) {
>   queue = >queues[index];
>   rx_bytes += queue->stats.rx_bytes;
>   rx_packets += queue->stats.rx_packets;
> @@ -241,6 +241,8 @@ static struct net_device_stats *xenvif_get_stats(struct 
> net_device *dev)
>   }
>  
>  out:
> + spin_unlock(>lock);
> +

Good catch, this is definitely needed. And it would probably be in a
separate patch.

Wei.

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

[Wei Liu]

On Tue, Jan 10, 2017 at 02:32:50PM +0100, Juergen Gross wrote:
> 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
> 

Acked-by: Wei Liu <wei.l...@citrix.com>

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

[Wei Liu]

On Fri, Jan 06, 2017 at 04:05:43PM +0100, 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>

Reviewed-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH] vif queue counters from int to long

[Wei Liu]

On Fri, Dec 23, 2016 at 04:09:23PM +0100, Mart van Santen wrote:
> 
> Hello,
> 
> This patch fixes an issue where counters in the queue have type int,
> while the counters of the vif itself are specified as long. This can
> cause incorrect reporting of tx/rx values of the vif interface.
> More extensively reported on xen-devel mailinglist.
> 

Hello,

Please also CC xen-de...@lists.xenproject.org for your future patch(es).
And please note that the most up to date maintainer information should
be used.

Wei.

> 
> 
> Signed-off-by: Mart van Santen 
> --- a/drivers/net/xen-netback/common.h  2016-12-22 15:41:07.785535748 +
> +++ b/drivers/net/xen-netback/common.h  2016-12-23 13:08:18.123080064 +
> @@ -113,10 +113,10 @@ struct xenvif_stats {
>  * A subset of struct net_device_stats that contains only the
>  * fields that are updated in netback.c for each queue.
>  */
> -   unsigned int rx_bytes;
> -   unsigned int rx_packets;
> -   unsigned int tx_bytes;
> -   unsigned int tx_packets;
> +   unsigned long rx_bytes;
> +   unsigned long rx_packets;
> +   unsigned long tx_bytes;
> +   unsigned long tx_packets;
> 
> /* Additional stats used by xenvif */
> unsigned long rx_gso_checksum_fixup;
> 
> -- 
> Mart van Santen
> Greenhost
> E: m...@greenhost.nl
> T: +31 20 4890444
> W: https://greenhost.nl
> 
> A PGP signature can be attached to this e-mail,
> you need PGP software to verify it. 
> My public key is available in keyserver(s)
> see: http://tinyurl.com/openpgp-manual
> 
> PGP Fingerprint: CA85 EB11 2B70 042D AF66  B29A 6437 01A1 10A3 D3A5
> 
> 

Re: [PATCH net-next v2 6/9] net: use core MTU range checking in virt drivers

[Wei Liu]

On Thu, Oct 20, 2016 at 01:55:21PM -0400, Jarod Wilson wrote:
> hyperv_net:
> - set min/max_mtu, per Haiyang, after rndis_filter_device_add
> 
> virtio_net:
> - set min/max_mtu
> - remove virtnet_change_mtu
> 
> vmxnet3:
> - set min/max_mtu
> 
> xen-netback:
> - min_mtu = 0, max_mtu = 65517
> 
> xen-netfront:
> - min_mtu = 0, max_mtu = 65535
> 
> unisys/visor:
> - clean up defines a little to not clash with network core or add
>   redundat definitions
> 
> CC: netdev@vger.kernel.org
> CC: virtualizat...@lists.linux-foundation.org
> CC: "K. Y. Srinivasan" <k...@microsoft.com>
> CC: Haiyang Zhang <haiya...@microsoft.com>
> CC: "Michael S. Tsirkin" <m...@redhat.com>
> CC: Shrikrishna Khare <skh...@vmware.com>
> CC: "VMware, Inc." <pv-driv...@vmware.com>
> CC: Wei Liu <wei.l...@citrix.com>
> CC: Paul Durrant <paul.durr...@citrix.com>
> CC: David Kershner <david.kersh...@unisys.com>
> Signed-off-by: Jarod Wilson <ja...@redhat.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH net] xen-netback: (re-)create a debugfs node for hash information

[Wei Liu]

On Mon, Oct 10, 2016 at 09:30:53AM +0100, Paul Durrant wrote:
> From: Paul Durrant <paul.durr...@citrix.com>
> 
> It is useful to be able to see the hash configuration when running tests.
> This patch adds a debugfs node for that purpose.
> 
> The original version of this patch (commit c0c64c152389) was reverted due
> to build failures caused by a conflict with commit 0364a8824c02
> ("xen-netback: switch to threaded irq for control ring"). This new version
> of the patch is nearly identical to the original, the only difference
> being that creation of the debugfs node is predicated on 'ctrl_irq' being
> non-zero rather then the now non-existent 'ctrl_task'.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>
> Cc: David S. Miller <da...@davemloft.net>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH net-next] MAINTAINERS: add myself as a maintainer of xen-netback

[Wei Liu]

On Fri, Oct 07, 2016 at 11:33:37AM +0100, Paul Durrant wrote:
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Thanks for stepping up!

> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 464437d..4491841 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13061,6 +13061,7 @@ F:arch/arm64/include/asm/xen/
>  
>  XEN NETWORK BACKEND DRIVER
>  M:   Wei Liu <wei.l...@citrix.com>
> +M:   Paul Durrant <paul.durr...@citrix.com>
>  L:   xen-de...@lists.xenproject.org (moderated for non-subscribers)
>  L:   netdev@vger.kernel.org
>  S:   Supported
> -- 
> 2.1.4
> 

Re: [PATCH v2 net] xen-netback: make sure that hashes are not send to unaware frontends

[Wei Liu]

On Fri, Oct 07, 2016 at 09:32:31AM +0100, Paul Durrant wrote:
> In the case when a frontend only negotiates a single queue with xen-
> netback it is possible for a skbuff with a s/w hash to result in a
> hash extra_info segment being sent to the frontend even when no hash
> algorithm has been configured. (The ndo_select_queue() entry point makes
> sure the hash is not set if no algorithm is configured, but this entry
> point is not called when there is only a single queue). This can result
> in a frontend that is unable to handle extra_info segments being given
> such a segment, causing it to crash.
> 
> This patch fixes the problem by clearing the hash in ndo_start_xmit()
> instead, which is clearly guaranteed to be called irrespective of the
> number of queues.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

> ---
> 
> v2:
>  - Simplified and re-based onto re-factored net branch
> ---
>  drivers/net/xen-netback/interface.c | 20 +---
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/interface.c 
> b/drivers/net/xen-netback/interface.c
> index 4af532a..74dc2bf 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -149,17 +149,8 @@ static u16 xenvif_select_queue(struct net_device *dev, 
> struct sk_buff *skb,
>   struct xenvif *vif = netdev_priv(dev);
>   unsigned int size = vif->hash.size;
>  
> - if (vif->hash.alg == XEN_NETIF_CTRL_HASH_ALGORITHM_NONE) {
> - u16 index = fallback(dev, skb) % dev->real_num_tx_queues;
> -
> - /* Make sure there is no hash information in the socket
> -  * buffer otherwise it would be incorrectly forwarded
> -  * to the frontend.
> -  */
> - skb_clear_hash(skb);
> -
> - return index;
> - }
> + if (vif->hash.alg == XEN_NETIF_CTRL_HASH_ALGORITHM_NONE)
> + return fallback(dev, skb) % dev->real_num_tx_queues;
>  
>   xenvif_set_skb_hash(vif, skb);
>  
> @@ -208,6 +199,13 @@ static int xenvif_start_xmit(struct sk_buff *skb, struct 
> net_device *dev)
>   cb = XENVIF_RX_CB(skb);
>   cb->expires = jiffies + vif->drain_timeout;
>  
> + /* If there is no hash algorithm configured then make sure there
> +  * is no hash information in the socket buffer otherwise it
> +  * would be incorrectly forwarded to the frontend.
> +  */
> + if (vif->hash.alg == XEN_NETIF_CTRL_HASH_ALGORITHM_NONE)
> + skb_clear_hash(skb);
> +
>   xenvif_rx_queue_tail(queue, skb);
>   xenvif_kick_thread(queue);
>  
> -- 
> 2.1.4
> 

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

[Wei Liu]

On Thu, Sep 22, 2016 at 11:06:25AM +0200, Juergen Gross wrote:
> Instead of open coding it use the threaded irq mechanism in
> xen-netback.
> 
> Signed-off-by: Juergen Gross <jgr...@suse.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH v2] xen-netback: fix error handling on netback_probe()

[Wei Liu]

On Thu, Sep 15, 2016 at 05:10:46PM +0200, Filipe Manco wrote:
> In case of error during netback_probe() (e.g. an entry missing on the
> xenstore) netback_remove() is called on the new device, which will set
> the device backend state to XenbusStateClosed by calling
> set_backend_state(). However, the backend state wasn't initialized by
> netback_probe() at this point, which will cause and invalid transaction
> and set_backend_state() to BUG().
> 
> Initialize the backend state at the beginning of netback_probe() to
> XenbusStateInitialising, and create two new valid state transitions on
> set_backend_state(), from XenbusStateInitialising to XenbusStateClosed,
> and from XenbusStateInitialising to XenbusStateInitWait.
> 
> Signed-off-by: Filipe Manco <filipe.ma...@neclab.eu>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [Xen-devel] [RFC PATCH] xen-netback: fix error handling on netback_probe()

[Wei Liu]

On Thu, Sep 15, 2016 at 04:05:17PM +0200, Filipe Manco wrote:
> On 14-09-2016 12:10, Wei Liu wrote:
> >CC xen-devel as well.
> >
> >On Tue, Sep 13, 2016 at 02:11:27PM +0200, Filipe Manco wrote:
> >>In case of error during netback_probe() (e.g. an entry missing on the
> >>xenstore) netback_remove() is called on the new device, which will set
> >>the device backend state to XenbusStateClosed by calling
> >>set_backend_state(). However, the backend state wasn't initialized by
> >>netback_probe() at this point, which will cause and invalid transaction
> >>and set_backend_state() to BUG().
> >>
> >>Initialize the backend state at the beginning of netback_probe() to
> >>XenbusStateInitialising, and create a new valid state transaction on
> >>set_backend_state(), from XenbusStateInitialising to XenbusStateClosed.
> >>
> >>Signed-off-by: Filipe Manco <filipe.ma...@neclab.eu>
> >There is a state machine right before set_backend_state. You would also
> >need to update that.
> Good point I'll update the diagram.
> 
> After looking at the diagram and for consistency, shouldn't the transition
> Initialising -> InitWait be handled using set_backend_state()? Currently it
> is done directly in netback_probe() code. If you agree I'll submit a v2 with
> these two changes.

That's fine with me.

Wei.

Re: [RFC PATCH] xen-netback: fix error handling on netback_probe()

[Wei Liu]

CC xen-devel as well.

On Tue, Sep 13, 2016 at 02:11:27PM +0200, Filipe Manco wrote:
> In case of error during netback_probe() (e.g. an entry missing on the
> xenstore) netback_remove() is called on the new device, which will set
> the device backend state to XenbusStateClosed by calling
> set_backend_state(). However, the backend state wasn't initialized by
> netback_probe() at this point, which will cause and invalid transaction
> and set_backend_state() to BUG().
> 
> Initialize the backend state at the beginning of netback_probe() to
> XenbusStateInitialising, and create a new valid state transaction on
> set_backend_state(), from XenbusStateInitialising to XenbusStateClosed.
> 
> Signed-off-by: Filipe Manco 

There is a state machine right before set_backend_state. You would also
need to update that.

According to the definition of XenbusStateInitialising, this patch looks
plausible to me.

Wei.

> ---
>  drivers/net/xen-netback/xenbus.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/xen-netback/xenbus.c 
> b/drivers/net/xen-netback/xenbus.c
> index 6a31f2610c23..c0e5f6994d01 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -270,6 +270,7 @@ static int netback_probe(struct xenbus_device *dev,
>  
>   be->dev = dev;
>   dev_set_drvdata(>dev, be);
> + be->state = XenbusStateInitialising;
>  
>   sg = 1;
>  
> @@ -515,6 +516,15 @@ static void set_backend_state(struct backend_info *be,
>  {
>   while (be->state != state) {
>   switch (be->state) {
> + case XenbusStateInitialising:
> + switch (state) {
> + case XenbusStateClosed:
> + backend_switch_state(be, XenbusStateClosed);
> + break;
> + default:
> + BUG();
> + }
> + break;
>   case XenbusStateClosed:
>   switch (state) {
>   case XenbusStateInitWait:
> -- 
> 2.7.4
> 

Re: [PATCH net-next] xen-netback: create a debugfs node for hash information

[Wei Liu]

On Wed, Aug 17, 2016 at 04:13:29PM +0100, Paul Durrant wrote:
> It is useful to be able to see the hash configuration when running tests.
> This patch adds a debugfs node for that purpose.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH 0805/1285] Replace numeric parameter like 0444 with macro

[Wei Liu]

On Tue, Aug 02, 2016 at 07:48:12PM +0800, Baole Ni wrote:
> I find that the developers often just specified the numeric value
> when calling a macro which is defined with a parameter for access permission.
> As we know, these numeric value for access permission have had the 
> corresponding macro,
> and that using macro can improve the robustness and readability of the code,
> thus, I suggest replacing the numeric parameter with the macro.
> 
> Signed-off-by: Chuansheng Liu <chuansheng@intel.com>
> Signed-off-by: Baole Ni <baolex...@intel.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH] xen-netback: prefer xenbus_write() over xenbus_printf() where possible

[Wei Liu]

On Fri, Jul 08, 2016 at 05:13:49PM +0100, Wei Liu wrote:
> On Thu, Jul 07, 2016 at 01:58:18AM -0600, Jan Beulich wrote:
> > ... as being the simpler variant.
> > 
> > Signed-off-by: Jan Beulich <jbeul...@suse.com>
> 
> Acked-by: Wei Liu <wei.l...@citrix.com>

Please ignore this, I acked v2 instead.

Only v2 is needed.

Re: [Xen-devel] [PATCH v2 4/4] xen-netback: prefer xenbus_scanf() over xenbus_gather()

[Wei Liu]

On Fri, Jul 08, 2016 at 06:28:58AM -0600, Jan Beulich wrote:
> For single items being collected this should be preferred as being more
> typesafe (as the compiler can check format string and to-be-written-to
> variable match) and more efficient (requiring one less parameter to be
> passed).
> 
> Signed-off-by: Jan Beulich <jbeul...@suse.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH] xen-netback: prefer xenbus_write() over xenbus_printf() where possible

[Wei Liu]

On Thu, Jul 07, 2016 at 01:58:18AM -0600, Jan Beulich wrote:
> ... as being the simpler variant.
> 
> Signed-off-by: Jan Beulich <jbeul...@suse.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [Xen-devel] [PATCH] xen-netback: correct return value checks on xenbus_scanf()

[Wei Liu]

On Thu, Jul 07, 2016 at 10:58:16AM +0100, David Vrabel wrote:
> On 07/07/16 08:57, Jan Beulich wrote:
> > Only a positive return value indicates success.
> 
> This is not correct.
> 

Do you mean the commit message is not correct or the code is not
correct? If it is the formal, do you have any suggestion to fix it?

(I was going to just ack this because Paul already reviewed it)

Wei.

> David

Re: [PATCH net-next] xen-netback: only deinitialized hash if it was initialized

[Wei Liu]

On Wed, May 18, 2016 at 03:55:42PM +0100, Paul Durrant wrote:
> A domain with a frontend that does not implement a control ring has been
> seen to cause a crash during domain save. This was apparently because
> the call to xenvif_deinit_hash() in xenvif_disconnect_ctrl() is made
> regardless of whether a control ring was connected, and hence
> xenvif_hash_init() was called.
> 
> This patch brings the call to xenvif_deinit_hash() in
> xenvif_disconnect_ctrl() inside the if clause that checks whether the
> control ring event channel was connected. This is sufficient to ensure
> it is only called if xenvif_init_hash() was called previously.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Reported-by: Boris Ostrovsky <boris.ostrov...@oracle.com>
> Tested-by: Boris Ostrovsky <boris.ostrov...@oracle.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

> ---
>  drivers/net/xen-netback/interface.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/interface.c 
> b/drivers/net/xen-netback/interface.c
> index 1c7f49b..83deeeb 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -780,9 +780,8 @@ void xenvif_disconnect_ctrl(struct xenvif *vif)
>   vif->ctrl_task = NULL;
>   }
>  
> - xenvif_deinit_hash(vif);
> -
>   if (vif->ctrl_irq) {
> + xenvif_deinit_hash(vif);
>   unbind_from_irqhandler(vif->ctrl_irq, vif);
>   vif->ctrl_irq = 0;
>   }
> -- 
> 2.1.4
> 

Re: [PATCH net-next] xen-netback: correct length checks on hash copy_ops

[Wei Liu]

On Wed, May 18, 2016 at 08:53:01AM +0100, Paul Durrant wrote:
> The length checks on the grant table copy_ops for setting hash key and
> hash mapping are checking the local 'len' value which is correct in
> the case of the former but not the latter. This was picked up by
> static analysis checks.
> 
> This patch replaces checks of 'len' with 'copy_op.len' in both cases
> to correct the incorrect check, keep the two checks consistent, and to
> make it clear what the checks are for.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

> ---
>  drivers/net/xen-netback/hash.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c
> index 392e392..fb87cb3 100644
> --- a/drivers/net/xen-netback/hash.c
> +++ b/drivers/net/xen-netback/hash.c
> @@ -311,7 +311,7 @@ u32 xenvif_set_hash_key(struct xenvif *vif, u32 gref, u32 
> len)
>   if (len > XEN_NETBK_MAX_HASH_KEY_SIZE)
>   return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
>  
> - if (len != 0) {
> + if (copy_op.len != 0) {
>   gnttab_batch_copy(_op, 1);
>  
>   if (copy_op.status != GNTST_okay)
> @@ -359,7 +359,7 @@ u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, 
> u32 len,
>   if (mapping[off++] >= vif->num_queues)
>   return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
>  
> - if (len != 0) {
> + if (copy_op.len != 0) {
>   gnttab_batch_copy(_op, 1);
>  
>   if (copy_op.status != GNTST_okay)
> -- 
> 2.1.4
> 

Re: [PATCH net-next v4 2/4] xen-netback: add control protocol implementation

[Wei Liu]

On Fri, May 13, 2016 at 09:37:27AM +0100, Paul Durrant wrote:
> My recent patch to include/xen/interface/io/netif.h defines a new shared
> ring (in addition to the rx and tx rings) for passing control messages
> from a VM frontend driver to a backend driver.
> 
> A previous patch added the necessary boilerplate for mapping the control
> ring from the frontend, should it be created. This patch adds
> implementations for each of the defined protocol messages.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH net] xen-netback: fix extra_info handling in xenvif_tx_err()

[Wei Liu]

On Thu, May 12, 2016 at 02:43:03PM +0100, Paul Durrant wrote:
> Patch 562abd39 "xen-netback: support multiple extra info fragments
> passed from frontend" contained a mistake which can result in an in-
> correct number of responses being generated when handling errors
> encountered when processing packets containing extra info fragments.
> This patch fixes the problem.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Reported-by: Jan Beulich <jbeul...@suse.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

> ---
>  drivers/net/xen-netback/netback.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/xen-netback/netback.c 
> b/drivers/net/xen-netback/netback.c
> index b42f260..4412a57 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -711,6 +711,7 @@ static void xenvif_tx_err(struct xenvif_queue *queue,
>   if (cons == end)
>   break;
>   RING_COPY_REQUEST(>tx, cons++, txp);
> + extra_count = 0; /* only the first frag can have extras */
>   } while (1);
>   queue->tx.req_cons = cons;
>  }
> -- 
> 2.1.4
> 

Re: [PATCH net-next v3 1/4] xen-netback: add control ring boilerplate

[Wei Liu]

On Wed, May 11, 2016 at 04:33:34PM +0100, Paul Durrant wrote:
> My recent patch to include/xen/interface/io/netif.h defines a new shared
> ring (in addition to the rx and tx rings) for passing control messages
> from a VM frontend driver to a backend driver.
> 
> This patch adds the necessary code to xen-netback to map this new shared
> ring, should it be created by a frontend, but does not add implementations
> for any of the defined protocol messages. These are added in a subsequent
> patch for clarity.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH net-next v3 2/4] xen-netback: add control protocol implementation

[Wei Liu]

On Wed, May 11, 2016 at 04:33:35PM +0100, Paul Durrant wrote:
> My recent patch to include/xen/interface/io/netif.h defines a new shared
> ring (in addition to the rx and tx rings) for passing control messages
> from a VM frontend driver to a backend driver.
> 
> A previous patch added the necessary boilerplate for mapping the control
> ring from the frontend, should it be created. This patch adds
> implementations for each of the defined protocol messages.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH net-next 4/4] xen-netback: use hash value from the frontend

[Wei Liu]

On Thu, May 05, 2016 at 12:19:30PM +0100, Paul Durrant wrote:
> My recent patch to include/xen/interface/io/netif.h defines a new extra
> info type that can be used to pass hash values between backend and guest
> frontend.
> 
> This patch adds code to xen-netback to use the value in a hash extra
> info fragment passed from the guest frontend in a transmit-side
> (i.e. netback receive side) packet to set the skb hash accordingly.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH net-next 3/4] xen-netback: pass hash value to the frontend

[Wei Liu]

On Thu, May 05, 2016 at 12:19:29PM +0100, Paul Durrant wrote:
> My recent patch to include/xen/interface/io/netif.h defines a new extra
> info type that can be used to pass hash values between backend and guest
> frontend.
> 
> This patch adds code to xen-netback to pass hash values calculated for
> guest receive-side packets (i.e. netback transmit side) to the frontend.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH net-next 2/4] xen-netback: add control protocol implementation

[Wei Liu]

On Thu, May 05, 2016 at 12:19:28PM +0100, Paul Durrant wrote:
> My recent patch to include/xen/interface/io/netif.h defines a new shared
> ring (in addition to the rx and tx rings) for passing control messages
> from a VM frontend driver to a backend driver.
> 
> A previous patch added the necessary boilerplate for mapping the control
> ring from the frontend, should it be created. This patch adds
> implementations for each of the defined protocol messages.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>
> ---
>  drivers/net/xen-netback/Makefile|   2 +-
>  drivers/net/xen-netback/common.h|  43 +
>  drivers/net/xen-netback/hash.c  | 361 
> 
>  drivers/net/xen-netback/interface.c |  28 +++
>  drivers/net/xen-netback/netback.c   |  49 -
>  5 files changed, 480 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/net/xen-netback/hash.c
> 

Other than the issue mentioned by David, the code looks OK to me.

Wei.

Re: [PATCH net-next 1/4] xen-netback: add control ring boilerplate

[Wei Liu]

On Thu, May 05, 2016 at 12:19:27PM +0100, Paul Durrant wrote:
[...]
>  
> +static int connect_ctrl_ring(struct backend_info *be)
> +{

Please use goto style error handling in this function.

Other than this the code looks good.

Wei.

Re: [PATCH net-next 1/3] xen-netback: re-import canonical netif header

[Wei Liu]

On Thu, Mar 10, 2016 at 12:30:26PM +, Paul Durrant wrote:
> The canonical netif header (in the Xen source repo) and the Linux variant
> have diverged significantly. Recently much documentation has been added to
> the canonical header which is highly useful for developers making
> modifications to either xen-netfront or xen-netback. This patch therefore
> re-imports the canonical header in its entirity.
> 
> To maintain compatibility and some style consistency with the old Linux
> variant, the header was stripped of its emacs boilerplate, and
> post-processed and copied into place with the following commands:
> 
> ed -s netif.h << EOF
> H
> ,s/NETTXF_/XEN_NETTXF_/g
> ,s/NETRXF_/XEN_NETRXF_/g
> ,s/NETIF_/XEN_NETIF_/g
> ,s/XEN_XEN_/XEN_/g
> ,s/netif/xen_netif/g
> ,s/xen_xen_/xen_/g
> ,s/^typedef.*$//g
> ,s/^/${TAB}/g
> w
> $
> w
> EOF
> 
> indent --line-length 80 --linux-style netif.h \
> -o include/xen/interface/io/netif.h
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrov...@oracle.com>
> Cc: David Vrabel <david.vra...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH net-next 2/3] xen-netback: support multiple extra info fragments passed from frontend

[Wei Liu]

On Thu, Mar 10, 2016 at 12:30:27PM +, Paul Durrant wrote:
> The code does not currently support a frontend passing multiple extra info
> fragments to the backend in a tx request. The xenvif_get_extras() function
> handles multiple extra_info fragments but make_tx_response() assumes there
> is only ever a single extra info fragment.
> 
> This patch modifies xenvif_get_extras() to pass back a count of extra
> info fragments, which is then passed to make_tx_response() (after
> possibly being stashed in pending_tx_info for deferred responses).
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [PATCH net-next 3/3] xen-netback: reduce log spam

[Wei Liu]

On Thu, Mar 10, 2016 at 12:30:28PM +, Paul Durrant wrote:
> Remove the "prepare for reconnect" pr_info in xenbus.c. It's largely
> uninteresting and the states of the frontend and backend can easily be
> observed by watching the (o)xenstored log.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

> ---
>  drivers/net/xen-netback/xenbus.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/xenbus.c 
> b/drivers/net/xen-netback/xenbus.c
> index 39a303d..bd182cd 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -511,8 +511,6 @@ static void set_backend_state(struct backend_info *be,
>   switch (state) {
>   case XenbusStateInitWait:
>   case XenbusStateConnected:
> - pr_info("%s: prepare for reconnect\n",
> - be->dev->nodename);
>   backend_switch_state(be, XenbusStateInitWait);
>   break;
>   case XenbusStateClosing:
> -- 
> 2.1.4
> 

Re: [PATCH] MAINTAINERS: Drop myself as xen netback maintainer

[Wei Liu]

On Fri, Feb 19, 2016 at 11:44:51AM +, Ian Campbell wrote:
> Wei has been picking this up for quite a while now.
> 
> Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

> ---
>  MAINTAINERS | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 28eb61b..ef30060 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12013,7 +12013,6 @@ F:arch/arm64/xen/
>  F:   arch/arm64/include/asm/xen/
>  
>  XEN NETWORK BACKEND DRIVER
> -M:   Ian Campbell <ian.campb...@citrix.com>
>  M:   Wei Liu <wei.l...@citrix.com>
>  L:   xen-de...@lists.xenproject.org (moderated for non-subscribers)
>  L:   netdev@vger.kernel.org
> -- 
> 2.1.4
> 

Re: [PATCH net-next v1] xen-netback: implement dynamic multicast control

[Wei Liu]

On Mon, Feb 01, 2016 at 02:40:53PM +, Paul Durrant wrote:
[...]
> +static int xen_register_mcast_ctrl_watch(struct xenbus_device *dev,
> +  struct xenvif *vif)
> +{
> + int err = 0;
> + char *node;
> + unsigned maxlen = strlen(dev->otherend) +
> + sizeof("/request-multicast-control");
> +
> + if (vif->mcast_ctrl_watch.node)
> + return -EADDRINUSE;
> +
> + node = kmalloc(maxlen, GFP_KERNEL);
> + if (!node)
> + return -ENOMEM;

This is one error path that has no logging, so please either add logging
in each error patch ...

> + snprintf(node, maxlen, "%s/request-multicast-control",
> +  dev->otherend);
> + vif->mcast_ctrl_watch.node = node;
> + vif->mcast_ctrl_watch.callback = xen_mcast_ctrl_changed;
> + err = register_xenbus_watch(>mcast_ctrl_watch);
> + if (err) {
> + pr_err("Failed to set watcher %s\n",
> +vif->mcast_ctrl_watch.node);
> + kfree(node);
> + vif->mcast_ctrl_watch.node = NULL;
> + vif->mcast_ctrl_watch.callback = NULL;
> + }
> + return err;
> +}
> +
> +static void xen_unregister_mcast_ctrl_watch(struct xenvif *vif)
> +{
> + if (vif->mcast_ctrl_watch.node) {
> + unregister_xenbus_watch(>mcast_ctrl_watch);
> + kfree(vif->mcast_ctrl_watch.node);
> + vif->mcast_ctrl_watch.node = NULL;
> + }
> +}
> +
> +static void xen_register_watchers(struct xenbus_device *dev,
> +   struct xenvif *vif)
> +{
> + xen_register_credit_watch(dev, vif);
> + xen_register_mcast_ctrl_watch(dev, vif);
> +}
> +
> +static void xen_unregister_watchers(struct xenvif *vif)
> +{
> + xen_unregister_mcast_ctrl_watch(vif);
> + xen_unregister_credit_watch(vif);
> +}

... or check if these register/unregister function calls' return value
and log if something fails.

Wei.

Re: [PATCH net-next v2] xen-netback: implement dynamic multicast control

[Wei Liu]

On Tue, Feb 02, 2016 at 11:31:08AM +, Paul Durrant wrote:
[...]
> +static int xen_register_mcast_ctrl_watch(struct xenbus_device *dev,
> +  struct xenvif *vif)
> +{
> + int err = 0;
> + char *node;
> + unsigned maxlen = strlen(dev->otherend) +
> + sizeof("/request-multicast-control");
> +
> + if (vif->mcast_ctrl_watch.node)
> + return -EADDRINUSE;
> +

Sorry if I didn't make myself clear enough. I think EADDRINUSE should
also be logged.

Wei.

Re: [PATCH net-next v3] xen-netback: implement dynamic multicast control

[Wei Liu]

On Tue, Feb 02, 2016 at 11:55:05AM +, Paul Durrant wrote:
> My recent patch to the Xen Project documents a protocol for 'dynamic
> multicast control' in netif.h. This extends the previous multicast control
> protocol to not require a shared ring reconnection to turn the feature off.
> Instead the backend watches the "request-multicast-control" key in xenstore
> and turns the feature off if the key value is written to zero.
> 
> This patch adds support for dynamic multicast control in xen-netback.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Ian Campbell <ian.campb...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>

Re: [Xen-devel] [PATCH 1/3] xen-netback: fix license ident used in MODULE_LICENSE

[Wei Liu]

On Fri, Jan 22, 2016 at 08:25:21PM +, One Thousand Gnomes wrote:
> > The fact what include/linux/license.h:license_is_gpl_compatible includes
> > "Dual MIT/GPL" as an option seems to suggest that it is enough of a thing
> > to be validly used as the contents of a MODULE_LICENSE() thing.
> 
> Yes. The MIT licence most definitely exists, and people know what it
> means.
> 
> Also nobody should be changing the licence on anything unless they have
> the written permission of all rights holders on record, so it's best to
> leave it be 8)
> 

I knew from the beginning anything related to license will be fun. :-)

In this particular case, I don't think I need to get confirmation from
all rights holder because they've agreed to the licenses listed in
the comment. I'm merely fixing a bug in code.

I understand people have different opinion on how this should be
interpreted. And I'm not a lawyer. Let's just leave it be now and divert
our energy to more useful things in life.

Wei.

> Alan

[PATCH 1/3] xen-netback: fix license ident used in MODULE_LICENSE

[Wei Liu]

The comment at the beginning of the file is the canonical source of
licenses for this module. Currently it contains GPL and MIT license. Fix
the code to reflect the reality.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
---
 drivers/net/xen-netback/netback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/xen-netback/netback.c 
b/drivers/net/xen-netback/netback.c
index 61b97c3..2427242 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -2192,5 +2192,5 @@ static void __exit netback_fini(void)
 }
 module_exit(netback_fini);
 
-MODULE_LICENSE("Dual BSD/GPL");
+MODULE_LICENSE("Dual MIT/GPL");
 MODULE_ALIAS("xen-backend:vif");
-- 
2.1.4

Re: [Xen-devel] [PATCH 1/3] xen-netback: fix license ident used in MODULE_LICENSE

[Wei Liu]

On Fri, Jan 22, 2016 at 01:14:24PM +, David Vrabel wrote:
> On 22/01/16 12:34, Wei Liu wrote:
> > The comment at the beginning of the file is the canonical source of
> > licenses for this module. Currently it contains GPL and MIT license. Fix
> > the code to reflect the reality.
> 
> "The MIT license" isn't really a thing.  The closest is the X11
> license[1], but this not applicable here either since the text in the
> drivers does not refer to X11 trademarks etc.
> 

That was referring to the license ident string in Linux.  If MIT license
isn't a thing, why would Linux have it at all?

> You can either use "GPL" which would be correct for a Linux kernel
> module since the alternate only applies when distributed separately from
> Linux ("or, when distributed separately from the Linux kernel or
> incorporated into other software packages, subject to the following
> license:"); or you can use "GPL and additional rights".
> 
> (Or you could just leave it as-is since "Dual BSD/GPL" is close enough.)
> 

No, I don't want to leave it as-is. That's not BSD license.

I can change that to "GPL". That is acceptable to me.

Wei.

> David
> 
> [1] http://www.gnu.org/licenses/license-list.html#X11License
> 

[PATCH v2 1/3] xen-netback: fix license ident used in MODULE_LICENSE

[Wei Liu]

The comment at the beginning of the file is the canonical source of
licenses for this module. The license used to distribute outside of
Linux kernel is not BSD license but X11 license. Instead of trying to
determine whether X11 license can be mapped into Linux's "MIT" license
ident, simply make the code to use "GPL" only.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
---
 drivers/net/xen-netback/netback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/xen-netback/netback.c 
b/drivers/net/xen-netback/netback.c
index 61b97c3..e9601d1 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -2192,5 +2192,5 @@ static void __exit netback_fini(void)
 }
 module_exit(netback_fini);
 
-MODULE_LICENSE("Dual BSD/GPL");
+MODULE_LICENSE("GPL");
 MODULE_ALIAS("xen-backend:vif");
-- 
2.1.4

Re: [PATCH 0/5] xen-netback: Fine-tuning for three function implementations

[Wei Liu]

I think the original code is fine.

Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/2] xen-netback: limit xen vif max queues number to online cpus

[Wei Liu]

On Fri, Oct 23, 2015 at 05:44:44PM +0800, Joe Jin wrote:
> Should not allocate xen vif queues number more than online cpus.

I think it's absolutely fine for administrators to override the value
should they choose to.

Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 6/8] xen-netback: pass an L4 or L3 skb hash value to the frontend

[Wei Liu]

On Wed, Oct 21, 2015 at 11:36:23AM +0100, Paul Durrant wrote:
> If the frontend indicates it's capable (see netif.h for details) and an
> skb has an L4 or L3 hash value then pass the value to the frontend in
> a xen_netif_extra_info segment.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Ian Campbell <ian.campb...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Reviewed-by: Wei Liu <wei.l...@citrix.com>

>  static int xenvif_rx_ring_slots_needed(struct xenvif *vif)
>  {
> - if (vif->gso_mask)
> - return DIV_ROUND_UP(vif->dev->gso_max_size, PAGE_SIZE) + 1;
> + int needed;
> +
> + if (vif->gso_mask || vif->gso_prefix_mask)

It seems like this line should become a patch for -stable?

>   xenvif_add_frag_responses(queue, status,
> diff --git a/drivers/net/xen-netback/xenbus.c 
> b/drivers/net/xen-netback/xenbus.c
> index 2fa8a16..a31bcee 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -1037,6 +1037,11 @@ static int read_xenbus_vif_flags(struct backend_info 
> *be)
>   val = 0;
>   vif->multicast_control = !!val;
>  
> + if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-hash",
> +  "%d", ) < 0)
> + val = 0;

Again, feel free to retain my reviewed-by if this changes in next
version.

Wei.

> + vif->hash_extra = !!val;
> +
>   return 0;
>  }
>  
> -- 
> 2.1.4
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 8/8] xen-netback: add support for toeplitz hashing

[Wei Liu]

On Wed, Oct 21, 2015 at 11:36:25AM +0100, Paul Durrant wrote:
> This patch adds all the necessary infrastructure to allow a frontend to
> specify toeplitz hashing of network packets on its receive side. (See
> netif.h for details of the xenbus protocol).
> 
> The toeplitz hash algorithm itself was based on pseudo-code provided by
> Microsoft at:
> 
> https://msdn.microsoft.com/en-us/library/windows/hardware/ff570725.aspx
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Ian Campbell <ian.campb...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>
[...]
>  
> diff --git a/drivers/net/xen-netback/interface.c 
> b/drivers/net/xen-netback/interface.c
> index 0c7da7b..38eee4f 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -142,17 +142,122 @@ void xenvif_wake_queue(struct xenvif_queue *queue)
>   netif_tx_wake_queue(netdev_get_tx_queue(dev, id));
>  }
>  

I skipped the hash implementation because I don't think I know enough to
tell if it is correct or not, and protocol negotiation because I think
that's going to change in next version.

> +
> +
> +static void xen_net_read_toeplitz_key(struct xenvif *vif,
> +   const char *node)
> +{
> + struct xenbus_device *dev = xenvif_to_xenbus_device(vif);
> + char *str, *token;
> + u8 key[40];

This should use the macro.

> + unsigned int n, i;
> +
> + str = xenbus_read(XBT_NIL, node, "key", NULL);
> + if (IS_ERR(str))
> + goto fail1;
> +
> + memset(key, 0, sizeof(key));
> +
> + n = 0;
> + while ((token = strsep(, ",")) != NULL) {
> + int rc;
> +
> + if (n >= ARRAY_SIZE(vif->hash_params.toeplitz.key)) {
> + pr_err("%s: key too big\n",
> +dev->nodename);
> + goto fail2;
> + }
> +
> + rc = kstrtou8(token, 0, [n]);
> + if (rc < 0) {
> + pr_err("%s: invalid key value (%s at index %u)\n",
> +dev->nodename, token, n);
> + goto fail2;
> + }
> +
> + n++;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(vif->hash_params.toeplitz.key); i++)
> + vif->hash_params.toeplitz.key[i] = key[i];
> +
> + kfree(str);
> + return;
> +
> +fail2:
> + kfree(str);
> +fail1:
> + vif->hash_params.toeplitz.types = 0;
> +}
> +
[...]
> +
> +static void xen_hash_changed(struct xenbus_watch *watch,
> +  const char **vec, unsigned int len)
> +{
> + struct xenvif *vif = container_of(watch, struct xenvif, hash_watch);
> +
> + xen_net_read_hash(vif);

I think the same question for previous patch applies here, too.

Is there any concern of correctness and security implication that you
just change the hash without stopping the vif?

Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 1/8] xen-netback: re-import canonical netif header

[Wei Liu]

On Wed, Oct 21, 2015 at 11:36:18AM +0100, Paul Durrant wrote:
> The canonical netif header (in the Xen source repo) and the Linux variant
> have diverged significantly. Recently much documentation has been added to
> the canonical header and new definitions and types to support packet hash
> configuration. Subsequent patches in this series add support for packet
> hash configuration in xen-netback so this patch re-imports the canonical
> header in readiness.
> 
> To maintain compatibility and some style consistency with the old Linux
> variant, the header was stripped of its emacs boilerplate, and
> post-processed and copied into place with the following commands:
> 
> ed -s netif.h << EOF
> H
> ,s/NETTXF_/XEN_NETTXF_/g
> ,s/NETRXF_/XEN_NETRXF_/g
> ,s/NETIF_RSP/XEN_NETIF_RSP/g
> ,s/netif_tx/xen_netif_tx/g
> ,s/netif_rx/xen_netif_rx/g
> ,s/netif_extra_info/xen_netif_extra_info/g
> w
> EOF
> 
> indent --linux-style netif.h -o include/xen/interface/io/netif.h
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrov...@oracle.com>
> Cc: David Vrabel <david.vra...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>
> ---
> 
> Whilst awaiting review of my patches to the canonical netif.h, import has
> been done from my staging branch using:
> 
> wget 
> http://xenbits.xen.org/gitweb/?p=people/pauldu/xen.git;a=blob_plain;f=xen/include/public/io/netif.h;hb=refs/heads/netif

There is on-going discussion on this so I'm going to skip this patch for
now.

Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 2/8] xen-netback: remove GSO information from xenvif_rx_meta

[Wei Liu]

On Wed, Oct 21, 2015 at 11:36:19AM +0100, Paul Durrant wrote:
> The code in net_rx_action() that builds rx responses has direct access
> to the skb so there is no need to copy this information into the meta
> structure.
> 
> This patch removes the extraneous fields, saves space in the array and
> removes many lines of code.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Ian Campbell <ian.campb...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Reviewed-by: Wei Liu <wei.l...@citrix.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 3/8] xen-netback: support multiple extra info segments passed from frontend

[Wei Liu]

On Wed, Oct 21, 2015 at 11:36:20AM +0100, Paul Durrant wrote:
> The code does not currently allow a frontend to pass multiple extra info
> segments to the backend in a tx request. A subsequent patch in this series
> needs this functionality so it is added here, without any other
> modification, for better bisectability.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Ian Campbell <ian.campb...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Reviewed-by: Wei Liu <wei.l...@citrix.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 4/8] xen-netback: accept an L4 or L3 skb hash value from the frontend

[Wei Liu]

On Wed, Oct 21, 2015 at 11:36:21AM +0100, Paul Durrant wrote:
> This patch adds an indication that netback is capable of handling hash
> values passed from the frontend (see netif.h for details), and the code
> necessary to process the additional xen_netif_extra_info segment and
> set a hash on the skb.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Ian Campbell <ian.campb...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Reviewed-by: Wei Liu <wei.l...@citrix.com>

[...]
>  
> + /* We support hash values. */
> + err = xenbus_printf(xbt, dev->nodename,
> + "feature-hash", "%d", 1);
> + if (err) {
> + message = "writing feature-hash";
> + goto abort_transaction;

Feel free to retain my reviewed-by if this changes in next version.

Wei.

> + }
> +
>   err = xenbus_transaction_end(xbt, 0);
>   } while (err == -EAGAIN);
>  
> -- 
> 2.1.4
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 7/8] xen-netback: add support for a multi-queue hash mapping table

[Wei Liu]

On Wed, Oct 21, 2015 at 11:36:24AM +0100, Paul Durrant wrote:
> Advertise the capability to handle a hash mapping specified by the
> frontend (see netif.h for details).
> 
> Add an ndo_select() entry point so that, of the frontend does specify a

"if the frontend ..."

> hash mapping, the skb hash is extracted and mapped to a queue. If no
> mapping is specified then the fallback queue selection function is
> called so there is no change in behaviour.
> 
> Signed-off-by: Paul Durrant 
[...]
> +static void xen_hash_mapping_changed(struct xenbus_watch *watch,
> +  const char **vec, unsigned int len)
> +{
> + struct xenvif *vif = container_of(watch, struct xenvif,
> +   hash_mapping_watch);
> +
> + xen_net_read_multi_queue_hash_mapping(vif);

Is it safe / correct to not stop the vif before changing mapping table?

Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen-netback: correctly check failed allocation

[Wei Liu]

On Thu, Oct 15, 2015 at 02:02:47PM -0400, Insu Yun wrote:
> I changed patch with valid format.
> 
> On Thu, Oct 15, 2015 at 2:02 PM, Insu Yun  wrote:
> 
> > Since vzalloc can be failed in memory pressure,
> > writes -ENOMEM to xenstore to indicate error.
> >
> > Signed-off-by: Insu Yun 
> > ---
> >  drivers/net/xen-netback/xenbus.c | 6 ++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/xen-netback/xenbus.c
> > b/drivers/net/xen-netback/xenbus.c
> > index 929a6e7..56ebd82 100644
> > --- a/drivers/net/xen-netback/xenbus.c
> > +++ b/drivers/net/xen-netback/xenbus.c
> > @@ -788,6 +788,12 @@ static void connect(struct backend_info *be)
> > /* Use the number of queues requested by the frontend */
> > be->vif->queues = vzalloc(requested_num_queues *
> >   sizeof(struct xenvif_queue));
> > +   if (!be->vif->queues) {
> > +   xenbus_dev_fatal(dev, -ENOMEM,
> > +"allocating queues");
> > +   return;
> >
> 
> I didn't use goto err, because another error handling is not required
> 

It's recommended in kernel coding style to use "goto" style error
handling. I personally prefer that to arbitrary return in function body,
too.

It's not a matter of whether another error handling is required or not,
it's about cleaner code that is easy to reason about and consistent
coding style.

The existing code is not perfect, but that doesn't mean we should follow
bad example.

Wei.

> 
> > +   }
> > +
> > be->vif->num_queues = requested_num_queues;
> > be->vif->stalled_queues = requested_num_queues;
> >
> > --
> > 1.9.1
> >
> >
> 
> 
> -- 
> Regards
> Insu Yun
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen-netback: correctly check failed allocation

[Wei Liu]

On Fri, Oct 16, 2015 at 10:05:21AM +0100, Wei Liu wrote:
> On Thu, Oct 15, 2015 at 02:02:47PM -0400, Insu Yun wrote:
> > I changed patch with valid format.
> > 
> > On Thu, Oct 15, 2015 at 2:02 PM, Insu Yun <wuni...@gmail.com> wrote:
> > 
> > > Since vzalloc can be failed in memory pressure,
> > > writes -ENOMEM to xenstore to indicate error.
> > >
> > > Signed-off-by: Insu Yun <wuni...@gmail.com>
> > > ---
> > >  drivers/net/xen-netback/xenbus.c | 6 ++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/net/xen-netback/xenbus.c
> > > b/drivers/net/xen-netback/xenbus.c
> > > index 929a6e7..56ebd82 100644
> > > --- a/drivers/net/xen-netback/xenbus.c
> > > +++ b/drivers/net/xen-netback/xenbus.c
> > > @@ -788,6 +788,12 @@ static void connect(struct backend_info *be)
> > > /* Use the number of queues requested by the frontend */
> > > be->vif->queues = vzalloc(requested_num_queues *
> > >   sizeof(struct xenvif_queue));
> > > +   if (!be->vif->queues) {
> > > +   xenbus_dev_fatal(dev, -ENOMEM,
> > > +"allocating queues");
> > > +   return;
> > >
> > 
> > I didn't use goto err, because another error handling is not required
> > 
> 
> It's recommended in kernel coding style to use "goto" style error
> handling. I personally prefer that to arbitrary return in function body,
> too.
> 
> It's not a matter of whether another error handling is required or not,
> it's about cleaner code that is easy to reason about and consistent
> coding style.
> 
> The existing code is not perfect, but that doesn't mean we should follow
> bad example.

And to be clear, I don't want to block this patch just because of this
coding style thing.  It's still an improvement and fix a real problem.
So:

Acked-by: Wei Liu <wei.l...@citrix.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] xen-netfront: update num_queues to real created

[Wei Liu]

CC David and Boris (Konrad was already a recipient).

On Thu, Oct 15, 2015 at 10:34:15AM +0800, Joe Jin wrote:
> Sometimes xennet_create_queues() may failed to created all requested
> queues, we need to update num_queues to real created to avoid NULL
> pointer dereference.
> 
> Signed-off-by: Joe Jin <joe@oracle.com>
> Cc: Wei Liu <wei.l...@citrix.com>
> Cc: Ian Campbell <ian.campb...@citrix.com>
> Cc: David S. Miller <da...@davemloft.net>
> ---
>  drivers/net/xen-netfront.c |9 ++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index f821a97..d580aec 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -1746,7 +1746,7 @@ static int xennet_create_queues(struct netfront_info 
> *info,
>   dev_err(>netdev->dev, "no queues\n");
>   return -EINVAL;
>   }
> - return 0;
> + return num_queues;
>  }
>  
>  /* Common code used when first setting up, and when resuming. */
> @@ -1788,9 +1788,12 @@ static int talk_to_netback(struct xenbus_device *dev,
>   if (info->queues)
>   xennet_destroy_queues(info);
>  
> - err = xennet_create_queues(info, num_queues);
> - if (err < 0)
> + /* Update queues number to real created */
> + num_queues = xennet_create_queues(info, num_queues);
> + if (num_queues < 0) {
> + err = num_queues;
>   goto destroy_ring;
> + }
>  
>   /* Create shared ring, alloc event channel -- for each queue */
>   for (i = 0; i < num_queues; ++i) {
> -- 
> 1.7.1
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen-netback: corretly check failed allocation

[Wei Liu]

On Thu, Oct 15, 2015 at 12:26:16PM -0400, Insu Yun wrote:
> Since vzalloc can be failed in memory pressure,
> return value should be checked and return ENOMEM.

This function doesn't return ENOMEM, instead it writes to xenstore to
indicate error. The commit log needs to be updated.

> 
> Signed-off-by: Insu Yun 
> ---
>  drivers/net/xen-netback/xenbus.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/xen-netback/xenbus.c 
> b/drivers/net/xen-netback/xenbus.c
> index 929a6e7..e288246 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -788,6 +788,11 @@ static void connect(struct backend_info *be)
>   /* Use the number of queues requested by the frontend */
>   be->vif->queues = vzalloc(requested_num_queues *
> sizeof(struct xenvif_queue));
> +  if (!be->vif->queues)  {
> +xenbus_dev_fatal(dev, -ENOMEM, "allocating queues");
> +return;
> +  }
> +

The indentation is wrong. Please configure your email client properly.

And please use "goto err" for error handling -- yes, I understand there
is existing code that returns directly but IMHO that should be fixed
too.

We.

>   be->vif->num_queues = requested_num_queues;
>   be->vif->stalled_queues = requested_num_queues;
>  
> -- 
> 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net v2 0/2] xen-net{front,back}: respect user provided max_queues

[Wei Liu]

On Wed, Sep 09, 2015 at 10:05:53PM -0700, David Miller wrote:
> From: David Miller <da...@davemloft.net>
> Date: Wed, 09 Sep 2015 21:53:22 -0700 (PDT)
> 
> > From: Wei Liu <wei.l...@citrix.com>
> > Date: Wed, 9 Sep 2015 11:23:04 +0100
> > 
> >> Wei Liu (2):
> >>   xen-netback: respect user provided max_queues
> >>   xen-netfront: respect user provided max_queues
> > 
> > Both applied, thanks.
> 
> Yo, this doesn't even compile!
> 
> drivers/net/xen-netfront.c: In function ‘netif_init’:
> drivers/net/xen-netfront.c:2138:6: error: ‘xenvif_max_queues’ undeclared 
> (first use in this function)
>   if (xenvif_max_queues == 0)
>   ^
> drivers/net/xen-netfront.c:2138:6: note: each undeclared identifier is 
> reported only once for each function it appears in
> 

Sorry about this.

I will send an updated version.

Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net v3 1/2] xen-netback: respect user provided max_queues

[Wei Liu]

Originally that parameter was always reset to num_online_cpus during
module initialisation, which renders it useless.

The fix is to only set max_queues to num_online_cpus when user has not
provided a value.

Reported-by: Johnny Strom <johnny.st...@linuxsolutions.fi>
Signed-off-by: Wei Liu <wei.l...@citrix.com>
Reviewed-by: David Vrabel <david.vra...@citrix.com>
Acked-by: Ian Campbell <ian.campb...@citrix.com>
---
 drivers/net/xen-netback/netback.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c 
b/drivers/net/xen-netback/netback.c
index 42569b9..f585c6a 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -2105,8 +2105,11 @@ static int __init netback_init(void)
if (!xen_domain())
return -ENODEV;
 
-   /* Allow as many queues as there are CPUs, by default */
-   xenvif_max_queues = num_online_cpus();
+   /* Allow as many queues as there are CPUs if user has not
+* specified a value.
+*/
+   if (xenvif_max_queues == 0)
+   xenvif_max_queues = 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.1.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net v3 0/2] xen-net{front,back}: respect user provided max_queues

[Wei Liu]

Wei Liu (2):
  xen-netback: respect user provided max_queues
  xen-netfront: respect user provided max_queues

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

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net v3 2/2] xen-netfront: respect user provided max_queues

[Wei Liu]

Originally that parameter was always reset to num_online_cpus during
module initialisation, which renders it useless.

The fix is to only set max_queues to num_online_cpus when user has not
provided a value.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Cc: David Vrabel <david.vra...@citrix.com>
---
v3: fix copy-n-paste error to make netfront compile
---
 drivers/net/xen-netfront.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index e27e6d2..b9c637a 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -2132,8 +2132,11 @@ static int __init netif_init(void)
 
pr_info("Initialising Xen virtual ethernet driver\n");
 
-   /* Allow as many queues as there are CPUs, by default */
-   xennet_max_queues = num_online_cpus();
+   /* Allow as many queues as there are CPUs if user has not
+* specified a value.
+*/
+   if (xennet_max_queues == 0)
+   xennet_max_queues = num_online_cpus();
 
return xenbus_register_frontend(_driver);
 }
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] xen-netback: respect user provided max_queues

[Wei Liu]

On Wed, Sep 09, 2015 at 11:12:44AM +0100, David Vrabel wrote:
> On 09/09/15 11:09, Wei Liu wrote:
> > Originally that parameter was always reset to num_online_cpus during
> > module initialisation, which renders it useless.
> > 
> > The fix is to only set max_queues to num_online_cpus when user has not
> > provided a value.
> [...]
> > --- a/drivers/net/xen-netback/netback.c
> > +++ b/drivers/net/xen-netback/netback.c
> > @@ -67,7 +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);
> >  
> > -unsigned int xenvif_max_queues;
> > +unsigned int xenvif_max_queues = 0;
> 
> You don't need this.
> 
> Otherwise,
> 
> Reviewed-by: David Vrabel <david.vra...@citrix.com>
> 
> Is an equivalent fix needed in netfront?
> 

I think so.

I will address your comment and send both fixes (front and back) in a
series.

Wei.

> David
> 
> >  module_param_named(max_queues, xenvif_max_queues, uint, 0644);
> >  MODULE_PARM_DESC(max_queues,
> >  "Maximum number of queues per virtual interface");
> > @@ -2105,8 +2105,11 @@ static int __init netback_init(void)
> > if (!xen_domain())
> > return -ENODEV;
> >  
> > -   /* Allow as many queues as there are CPUs, by default */
> > -   xenvif_max_queues = num_online_cpus();
> > +   /* Allow as many queues as there are CPUs if user has not
> > +* specified a value.
> > +*/
> > +   if (xenvif_max_queues == 0)
> > +   xenvif_max_queues = 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",
> > 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net v2 2/2] xen-netfront: respect user provided max_queues

[Wei Liu]

Originally that parameter was always reset to num_online_cpus during
module initialisation, which renders it useless.

The fix is to only set max_queues to num_online_cpus when user has not
provided a value.

Signed-off-by: Wei Liu <wei.l...@citrix.com>
Cc: David Vrabel <david.vra...@citrix.com>
---
 drivers/net/xen-netfront.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index e27e6d2..0b53bd9 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -2132,8 +2132,11 @@ static int __init netif_init(void)
 
pr_info("Initialising Xen virtual ethernet driver\n");
 
-   /* Allow as many queues as there are CPUs, by default */
-   xennet_max_queues = num_online_cpus();
+   /* Allow as many queues as there are CPUs if user has not
+* specified a value.
+*/
+   if (xenvif_max_queues == 0)
+   xenvif_max_queues = num_online_cpus();
 
return xenbus_register_frontend(_driver);
 }
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net v2 1/2] xen-netback: respect user provided max_queues

[Wei Liu]

Originally that parameter was always reset to num_online_cpus during
module initialisation, which renders it useless.

The fix is to only set max_queues to num_online_cpus when user has not
provided a value.

Reported-by: Johnny Strom <johnny.st...@linuxsolutions.fi>
Signed-off-by: Wei Liu <wei.l...@citrix.com>
Reviewed-by: David Vrabel <david.vra...@citrix.com>
Cc: Ian Campbell <ian.campb...@citrix.com>
---
Cc: Johnny Strom <johnny.st...@linuxsolutions.fi>
---
 drivers/net/xen-netback/netback.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c 
b/drivers/net/xen-netback/netback.c
index 42569b9..f585c6a 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -2105,8 +2105,11 @@ static int __init netback_init(void)
if (!xen_domain())
return -ENODEV;
 
-   /* Allow as many queues as there are CPUs, by default */
-   xenvif_max_queues = num_online_cpus();
+   /* Allow as many queues as there are CPUs if user has not
+* specified a value.
+*/
+   if (xenvif_max_queues == 0)
+   xenvif_max_queues = 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.1.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net v2 0/2] xen-net{front,back}: respect user provided max_queues

[Wei Liu]

Wei Liu (2):
  xen-netback: respect user provided max_queues
  xen-netfront: respect user provided max_queues

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

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv1 net] xen-netback: require fewer guest Rx slots when not using GSO

[Wei Liu]

On Tue, Sep 08, 2015 at 02:25:14PM +0100, David Vrabel wrote:
> Commit f48da8b14d04ca87ffcffe68829afd45f926ec6a (xen-netback: fix
> unlimited guest Rx internal queue and carrier flapping) introduced a
> regression.
> 
> The PV frontend in IPXE only places 4 requests on the guest Rx ring.
> Since netback required at least (MAX_SKB_FRAGS + 1) slots, IPXE could
> not receive any packets.
> 
> a) If GSO is not enabled on the VIF, fewer guest Rx slots are required
>for the largest possible packet.  Calculate the required slots
>based on the maximum GSO size or the MTU.
> 
>This calculation of the number of required slots relies on
>1650d5455bd2 (xen-netback: always fully coalesce guest Rx packets)
>which present in 4.0-rc1 and later.
> 
> b) Reduce the Rx stall detection to checking for at least one
>available Rx request.  This is fine since we're predominately
>concerned with detecting interfaces which are down and thus have
>zero available Rx requests.
> 
> Signed-off-by: David Vrabel <david.vra...@citrix.com>

Reviewed-by: Wei Liu <wei.l...@citrix.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net] xen-netback: respect user provided max_queues

[Wei Liu]

Originally that parameter was always reset to num_online_cpus during
module initialisation, which renders it useless.

The fix is to only set max_queues to num_online_cpus when user has not
provided a value.

Reported-by: Johnny Strom <johnny.st...@linuxsolutions.fi>
Signed-off-by: Wei Liu <wei.l...@citrix.com>
Cc: Ian Campbell <ian.campb...@citrix.com>
---
Cc: Johnny Strom <johnny.st...@linuxsolutions.fi>
Cc: David Vrabel <david.vra...@citrix.com>
---
 drivers/net/xen-netback/netback.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c 
b/drivers/net/xen-netback/netback.c
index 42569b9..b219a80 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -67,7 +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);
 
-unsigned int xenvif_max_queues;
+unsigned int xenvif_max_queues = 0;
 module_param_named(max_queues, xenvif_max_queues, uint, 0644);
 MODULE_PARM_DESC(max_queues,
 "Maximum number of queues per virtual interface");
@@ -2105,8 +2105,11 @@ static int __init netback_init(void)
if (!xen_domain())
return -ENODEV;
 
-   /* Allow as many queues as there are CPUs, by default */
-   xenvif_max_queues = num_online_cpus();
+   /* Allow as many queues as there are CPUs if user has not
+* specified a value.
+*/
+   if (xenvif_max_queues == 0)
+   xenvif_max_queues = 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.1.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 18/20] net/xen-netback: Make it running on 64KB page granularity

[Wei Liu]

You might need to rebase you patch. A patch to netback went it recently.

On Mon, Sep 07, 2015 at 04:33:56PM +0100, Julien Grall wrote:
> The PV network protocol is using 4KB page granularity. The goal of this
> patch is to allow a Linux using 64KB page granularity working as a
> network backend on a non-modified Xen.
> 
> It's only necessary to adapt the ring size and break skb data in small
> chunk of 4KB. The rest of the code is relying on the grant table code.
> 
> Signed-off-by: Julien Grall <julien.gr...@citrix.com>
> 

Reviewed-by: Wei Liu <wei.l...@citrix.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 net-next] xen-netback: add support for multicast control

[Wei Liu]

On Wed, Sep 02, 2015 at 05:58:36PM +0100, Paul Durrant wrote:
> Xen's PV network protocol includes messages to add/remove ethernet
> multicast addresses to/from a filter list in the backend. This allows
> the frontend to request the backend only forward multicast packets
> which are of interest thus preventing unnecessary noise on the shared
> ring.
> 
> The canonical netif header in git://xenbits.xen.org/xen.git specifies
> the message format (two more XEN_NETIF_EXTRA_TYPEs) so the minimal
> necessary changes have been pulled into include/xen/interface/io/netif.h.
> 
> To prevent the frontend from extending the multicast filter list
> arbitrarily a limit (XEN_NETBK_MCAST_MAX) has been set to 64 entries.
> This limit is not specified by the protocol and so may change in future.
> If the limit is reached then the next XEN_NETIF_EXTRA_TYPE_MCAST_ADD
> sent by the frontend will be failed with NETIF_RSP_ERROR.
> 
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> Cc: Ian Campbell <ian.campb...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>

Acked-by: Wei Liu <wei.l...@citrix.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] xen-netback: add support for multicast control

[Wei Liu]

On Wed, Sep 02, 2015 at 01:19:53PM +0100, Paul Durrant wrote:
> Xen's PV network protocol includes messages to add/remove ethernet
> multicast addresses to/from a filter list in the backend. This allows
> the frontend to request the backend only forward multicast packets
> which are off interest thus preventing unnecessary noise on the shared

"of interest"

> ring.
> 
[...]
> +
> +void xenvif_mcast_flush(struct xenvif *vif)

Only one cosmetic comment.

My first impression of this function by looking at the name is that it
flushes queued multicast packets. Maybe we can rename it to
xenvif_mcast_addr_list_free ?

Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 01/20] net/xen-netback: xenvif_gop_frag_copy: move GSO check out of the loop

[Wei Liu]

On Fri, Aug 07, 2015 at 05:46:40PM +0100, Julien Grall wrote:
 The skb doesn't change within the function. Therefore it's only
 necessary to check if we need GSO once at the beginning.
 
 Signed-off-by: Julien Grall julien.gr...@citrix.com
 

Acked-by: Wei Liu wei.l...@citrix.com
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 18/20] net/xen-netback: Make it running on 64KB page granularity

[Wei Liu]

On Fri, Aug 07, 2015 at 05:46:57PM +0100, Julien Grall wrote:
 The PV network protocol is using 4KB page granularity. The goal of this
 patch is to allow a Linux using 64KB page granularity working as a
 network backend on a non-modified Xen.
 
 It's only necessary to adapt the ring size and break skb data in small
 chunk of 4KB. The rest of the code is relying on the grant table code.
 
 Signed-off-by: Julien Grall julien.gr...@citrix.com
 
 ---
 Cc: Ian Campbell ian.campb...@citrix.com
 Cc: Wei Liu wei.l...@citrix.com
 Cc: netdev@vger.kernel.org
 
[...]
 +#define XEN_NETIF_TX_RING_SIZE __CONST_RING_SIZE(xen_netif_tx, XEN_PAGE_SIZE)
 +#define XEN_NETIF_RX_RING_SIZE __CONST_RING_SIZE(xen_netif_rx, XEN_PAGE_SIZE)
  
  struct xenvif_rx_meta {
   int id;
 @@ -80,16 +81,18 @@ struct xenvif_rx_meta {
  /* Discriminate from any valid pending_idx value. */
  #define INVALID_PENDING_IDX 0x
  
 -#define MAX_BUFFER_OFFSET PAGE_SIZE
 +#define MAX_BUFFER_OFFSET XEN_PAGE_SIZE
  
  #define MAX_PENDING_REQS XEN_NETIF_TX_RING_SIZE
  
 +#define MAX_XEN_SKB_FRAGS (65536 / XEN_PAGE_SIZE + 1)
 +

It might be clearer if you add a comment saying the maximum number of
frags is derived from the page size of the grant page, which happens to
be XEN_PAGE_SIZE at the moment. 

In the future we need to figure out the page size of grant page in a
dynamic way. We shall cross the bridge when we get there.

  /* It's possible for an skb to have a maximal number of frags
   * but still be less than MAX_BUFFER_OFFSET in size. Thus the
 - * worst-case number of copy operations is MAX_SKB_FRAGS per
 + * worst-case number of copy operations is MAX_XEN_SKB_FRAGS per
   * ring slot.
   */
 -#define MAX_GRANT_COPY_OPS (MAX_SKB_FRAGS * XEN_NETIF_RX_RING_SIZE)
 +#define MAX_GRANT_COPY_OPS (MAX_XEN_SKB_FRAGS * XEN_NETIF_RX_RING_SIZE)
  
  #define NETBACK_INVALID_HANDLE -1
  
 @@ -203,7 +206,7 @@ struct xenvif_queue { /* Per-queue data for xenvif */
  /* Maximum number of Rx slots a to-guest packet may use, including the
   * slot needed for GSO meta-data.
   */
 -#define XEN_NETBK_RX_SLOTS_MAX (MAX_SKB_FRAGS + 1)
 +#define XEN_NETBK_RX_SLOTS_MAX ((MAX_XEN_SKB_FRAGS + 1))
  
  enum state_bit_shift {
   /* This bit marks that the vif is connected */
 diff --git a/drivers/net/xen-netback/netback.c 
 b/drivers/net/xen-netback/netback.c
 index 66f1780..c32a9f2 100644
 --- a/drivers/net/xen-netback/netback.c
 +++ b/drivers/net/xen-netback/netback.c
 @@ -263,6 +263,80 @@ static struct xenvif_rx_meta *get_next_rx_buffer(struct 
 xenvif_queue *queue,
   return meta;
  }
  
[...]
   * Set up the grant operations for this fragment. If it's a flipping
   * interface, we also set up the unmap request from here.
 @@ -272,83 +346,52 @@ static void xenvif_gop_frag_copy(struct xenvif_queue 
 *queue, struct sk_buff *skb
struct page *page, unsigned long size,
unsigned long offset, int *head)
  {
 - struct gnttab_copy *copy_gop;
 - struct xenvif_rx_meta *meta;
 + struct gop_frag_copy info = {
 + .queue = queue,
 + .npo = npo,
 + .head = *head,
 + .gso_type = XEN_NETIF_GSO_TYPE_NONE,
 + };
   unsigned long bytes;
 - int gso_type = XEN_NETIF_GSO_TYPE_NONE;
  
   if (skb_is_gso(skb)) {
   if (skb_shinfo(skb)-gso_type  SKB_GSO_TCPV4)
 - gso_type = XEN_NETIF_GSO_TYPE_TCPV4;
 + info.gso_type = XEN_NETIF_GSO_TYPE_TCPV4;
   else if (skb_shinfo(skb)-gso_type  SKB_GSO_TCPV6)
 - gso_type = XEN_NETIF_GSO_TYPE_TCPV6;
 + info.gso_type = XEN_NETIF_GSO_TYPE_TCPV6;
   }
  
   /* Data must not cross a page boundary. */
   BUG_ON(size + offset  PAGE_SIZEcompound_order(page));
  
 - meta = npo-meta + npo-meta_prod - 1;
 + info.meta = npo-meta + npo-meta_prod - 1;
  
   /* Skip unused frames from start of page */
   page += offset  PAGE_SHIFT;
   offset = ~PAGE_MASK;
  
   while (size  0) {
 - struct xen_page_foreign *foreign;
 -
   BUG_ON(offset = PAGE_SIZE);
 - BUG_ON(npo-copy_off  MAX_BUFFER_OFFSET);
 -
 - if (npo-copy_off == MAX_BUFFER_OFFSET)
 - meta = get_next_rx_buffer(queue, npo);
  
   bytes = PAGE_SIZE - offset;
   if (bytes  size)
   bytes = size;
  
 - if (npo-copy_off + bytes  MAX_BUFFER_OFFSET)
 - bytes = MAX_BUFFER_OFFSET - npo-copy_off;
 -
 - copy_gop = npo-copy + npo-copy_prod++;
 - copy_gop-flags = GNTCOPY_dest_gref;
 - copy_gop-len = bytes;
 -
 - foreign = xen_page_foreign(page);
 - if (foreign) {
 - copy_gop-source.domid = foreign-domid;
 - copy_gop-source.u.ref = foreign-gref;
 - copy_gop-flags |= GNTCOPY_source_gref

Re: [PATCH v2 4/8] xen: Use the correctly the Xen memory terminologies

[Wei Liu]

On Tue, Aug 04, 2015 at 07:12:48PM +0100, Julien Grall wrote:
[...]
 diff --git a/drivers/net/xen-netback/netback.c 
 b/drivers/net/xen-netback/netback.c
 index 7d50711..3b7b7c3 100644
 --- a/drivers/net/xen-netback/netback.c
 +++ b/drivers/net/xen-netback/netback.c
 @@ -314,7 +314,7 @@ static void xenvif_gop_frag_copy(struct xenvif_queue 
 *queue, struct sk_buff *skb
   } else {
   copy_gop-source.domid = DOMID_SELF;
   copy_gop-source.u.gmfn =
 - virt_to_mfn(page_address(page));
 + virt_to_gfn(page_address(page));
   }
   copy_gop-source.offset = offset;
  
 @@ -1284,7 +1284,7 @@ static void xenvif_tx_build_gops(struct xenvif_queue 
 *queue,
   queue-tx_copy_ops[*copy_ops].source.offset = txreq.offset;
  
   queue-tx_copy_ops[*copy_ops].dest.u.gmfn =
 - virt_to_mfn(skb-data);
 + virt_to_gfn(skb-data);
   queue-tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
   queue-tx_copy_ops[*copy_ops].dest.offset =
   offset_in_page(skb-data);

Acked-by: Wei Liu wei.l...@citrix.com
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xen/netback: Wake dealloc thread after completing zerocopy work

[Wei Liu]

On Tue, Aug 04, 2015 at 03:40:59PM +0100, Ross Lagerwall wrote:
 Waking the dealloc thread before decrementing inflight_packets is racy
 because it means the thread may go to sleep before inflight_packets is
 decremented. If kthread_stop() has already been called, the dealloc
 thread may wait forever with nothing to wake it. Instead, wake the
 thread only after decrementing inflight_packets.
 
 Signed-off-by: Ross Lagerwall ross.lagerw...@citrix.com

Acked-by: Wei Liu wei.l...@citrix.com

 ---
 Changed in V2: Move wakeup into zerocopy_complete function.
 
  drivers/net/xen-netback/interface.c | 6 ++
  drivers/net/xen-netback/netback.c   | 1 -
  2 files changed, 6 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/net/xen-netback/interface.c 
 b/drivers/net/xen-netback/interface.c
 index 1a83e19..28577a3 100644
 --- a/drivers/net/xen-netback/interface.c
 +++ b/drivers/net/xen-netback/interface.c
 @@ -61,6 +61,12 @@ void xenvif_skb_zerocopy_prepare(struct xenvif_queue 
 *queue,
  void xenvif_skb_zerocopy_complete(struct xenvif_queue *queue)
  {
   atomic_dec(queue-inflight_packets);
 +
 + /* Wake the dealloc thread _after_ decrementing inflight_packets so
 +  * that if kthread_stop() has already been called, the dealloc thread
 +  * does not wait forever with nothing to wake it.
 +  */
 + wake_up(queue-dealloc_wq);
  }
  
  int xenvif_schedulable(struct xenvif *vif)
 diff --git a/drivers/net/xen-netback/netback.c 
 b/drivers/net/xen-netback/netback.c
 index 7d50711..09ffda4 100644
 --- a/drivers/net/xen-netback/netback.c
 +++ b/drivers/net/xen-netback/netback.c
 @@ -1536,7 +1536,6 @@ void xenvif_zerocopy_callback(struct ubuf_info *ubuf, 
 bool zerocopy_success)
   smp_wmb();
   queue-dealloc_prod++;
   } while (ubuf);
 - wake_up(queue-dealloc_wq);
   spin_unlock_irqrestore(queue-callback_lock, flags);
  
   if (likely(zerocopy_success))
 -- 
 2.1.0
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen/netback: Wake dealloc thread after completing zerocopy work

[Wei Liu]

On Tue, Aug 04, 2015 at 01:50:58PM +0100, Ross Lagerwall wrote:
 Waking the dealloc thread before decrementing inflight_packets is racy
 because it means the thread may go to sleep before inflight_packets is
 decremented. If kthread_stop() has already been called, the dealloc
 thread may wait forever with nothing to wake it. Instead, wake the
 thread only after decrementing inflight_packets.
 
 Signed-off-by: Ross Lagerwall ross.lagerw...@citrix.com
 ---
  drivers/net/xen-netback/netback.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/net/xen-netback/netback.c 
 b/drivers/net/xen-netback/netback.c
 index 7d50711..e95ee20 100644
 --- a/drivers/net/xen-netback/netback.c
 +++ b/drivers/net/xen-netback/netback.c
 @@ -1536,7 +1536,6 @@ void xenvif_zerocopy_callback(struct ubuf_info *ubuf, 
 bool zerocopy_success)
   smp_wmb();
   queue-dealloc_prod++;
   } while (ubuf);
 - wake_up(queue-dealloc_wq);
   spin_unlock_irqrestore(queue-callback_lock, flags);
  
   if (likely(zerocopy_success))
 @@ -1544,6 +1543,7 @@ void xenvif_zerocopy_callback(struct ubuf_info *ubuf, 
 bool zerocopy_success)
   else
   queue-stats.tx_zerocopy_fail++;
   xenvif_skb_zerocopy_complete(queue);
 + wake_up(queue-dealloc_wq);

Can you move this wake_up into xenvif_skb_zerocopy_complete and have a
comment there saying wake_up must be called after decrementing inflight
counters (possibly with some of the commit message)? That way we don't
trip over this in the future if we're to refactor the callback function.

Wei.

  }
  
  static inline void xenvif_tx_dealloc_action(struct xenvif_queue *queue)
 -- 
 2.1.0
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/8] xen: Use the correctly the Xen memory terminologies

[Wei Liu]

On Tue, Jul 28, 2015 at 04:02:45PM +0100, Julien Grall wrote:
[...]
 diff --git a/drivers/net/xen-netback/netback.c 
 b/drivers/net/xen-netback/netback.c
 index 7d50711..3b7b7c3 100644
 --- a/drivers/net/xen-netback/netback.c
 +++ b/drivers/net/xen-netback/netback.c
 @@ -314,7 +314,7 @@ static void xenvif_gop_frag_copy(struct xenvif_queue 
 *queue, struct sk_buff *skb
   } else {
   copy_gop-source.domid = DOMID_SELF;
   copy_gop-source.u.gmfn =
 - virt_to_mfn(page_address(page));
 + virt_to_gfn(page_address(page));
   }
   copy_gop-source.offset = offset;
  
 @@ -1284,7 +1284,7 @@ static void xenvif_tx_build_gops(struct xenvif_queue 
 *queue,
   queue-tx_copy_ops[*copy_ops].source.offset = txreq.offset;
  
   queue-tx_copy_ops[*copy_ops].dest.u.gmfn =
 - virt_to_mfn(skb-data);
 + virt_to_gfn(skb-data);
   queue-tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
   queue-tx_copy_ops[*copy_ops].dest.offset =
   offset_in_page(skb-data);

Reviewed-by: Wei Liu wei.l...@citrix.com

One possible improvement is to change gmfn in copy_gop to gfn as well.
But that's outside of netback code.

Wei.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH 4/8] xen: Use the correctly the Xen memory terminologies

[Wei Liu]

On Wed, Jul 29, 2015 at 12:35:54PM +0100, Julien Grall wrote:
 Hi Wei,
 
 On 29/07/15 11:13, Wei Liu wrote:
  On Tue, Jul 28, 2015 at 04:02:45PM +0100, Julien Grall wrote:
  [...]
  diff --git a/drivers/net/xen-netback/netback.c 
  b/drivers/net/xen-netback/netback.c
  index 7d50711..3b7b7c3 100644
  --- a/drivers/net/xen-netback/netback.c
  +++ b/drivers/net/xen-netback/netback.c
  @@ -314,7 +314,7 @@ static void xenvif_gop_frag_copy(struct xenvif_queue 
  *queue, struct sk_buff *skb
 } else {
 copy_gop-source.domid = DOMID_SELF;
 copy_gop-source.u.gmfn =
  -  virt_to_mfn(page_address(page));
  +  virt_to_gfn(page_address(page));
 }
 copy_gop-source.offset = offset;
   
  @@ -1284,7 +1284,7 @@ static void xenvif_tx_build_gops(struct xenvif_queue 
  *queue,
 queue-tx_copy_ops[*copy_ops].source.offset = txreq.offset;
   
 queue-tx_copy_ops[*copy_ops].dest.u.gmfn =
  -  virt_to_mfn(skb-data);
  +  virt_to_gfn(skb-data);
 queue-tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
 queue-tx_copy_ops[*copy_ops].dest.offset =
 offset_in_page(skb-data);
  
  Reviewed-by: Wei Liu wei.l...@citrix.com
  
  One possible improvement is to change gmfn in copy_gop to gfn as well.
  But that's outside of netback code.
 
 The structure gnttab_copy is part of the hypervisor interface. Is it
 fine to differ on the naming between Xen and Linux?
 
 Or maybe we could do the change in the public headers in Xen repo too.
 Is it fine to do field renaming in public headers?
 

Oh well. Never mind then. I mistook that structure as internal to Linux.

Wei.

 Regards,
 
 -- 
 Julien Grall
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen-netback: remove duplicated function definition

[Wei Liu]

On Sat, Jul 04, 2015 at 03:33:00AM +0800, Liang Li wrote:
 There are two duplicated xenvif_zerocopy_callback() definitions.
 Remove one of them.
 
 Signed-off-by: Liang Li liang.z...@intel.com

Acked-by: Wei Liu wei.l...@citrix.com

Please fix the time of your computer and resend.

Wei.

 ---
  drivers/net/xen-netback/common.h | 3 ---
  1 file changed, 3 deletions(-)
 
 diff --git a/drivers/net/xen-netback/common.h 
 b/drivers/net/xen-netback/common.h
 index 8a495b3..c6cb85a 100644
 --- a/drivers/net/xen-netback/common.h
 +++ b/drivers/net/xen-netback/common.h
 @@ -325,9 +325,6 @@ static inline pending_ring_idx_t nr_pending_reqs(struct 
 xenvif_queue *queue)
   queue-pending_prod + queue-pending_cons;
  }
  
 -/* Callback from stack when TX packet can be released */
 -void xenvif_zerocopy_callback(struct ubuf_info *ubuf, bool zerocopy_success);
 -
  irqreturn_t xenvif_interrupt(int irq, void *dev_id);
  
  extern bool separate_tx_rx_irq;
 -- 
 1.9.1
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen-netback: fix a BUG() during initialization

[Wei Liu]

On Fri, Jun 19, 2015 at 02:21:51PM +0200, Imre Palik wrote:
 From: Palik, Imre im...@amazon.de
 
 Commit edafc132baac (xen-netback: making the bandwidth limiter runtime 
 settable)
 introduced the capability to change the bandwidth rate limit at runtime.
 But it also introduced a possible crashing bug.
 
 If netback receives two XenbusStateConnected without getting the
 hotplug-status watch firing in between, then it will try to register the
 watches for the rate limiter again.  But this triggers a BUG() in the watch
 registration code.
 
 The fix modifies connect() to remove the possibly existing packet-rate
 watches before trying to install those watches.  This behaviour is in line
 with how connect() deals with the hotplug-status watch.
 
 Signed-off-by: Imre Palik im...@amazon.de
 Cc: Matt Wilson m...@amazon.com

Acked-by: Wei Liu wei.l...@citrix.com

 ---
  drivers/net/xen-netback/xenbus.c |4 
  1 file changed, 4 insertions(+)
 
 diff --git a/drivers/net/xen-netback/xenbus.c 
 b/drivers/net/xen-netback/xenbus.c
 index 968787a..ec383b0 100644
 --- a/drivers/net/xen-netback/xenbus.c
 +++ b/drivers/net/xen-netback/xenbus.c
 @@ -681,6 +681,9 @@ static int xen_register_watchers(struct xenbus_device 
 *dev, struct xenvif *vif)
   char *node;
   unsigned maxlen = strlen(dev-nodename) + sizeof(/rate);
  
 + if (vif-credit_watch.node)
 + return -EADDRINUSE;
 +
   node = kmalloc(maxlen, GFP_KERNEL);
   if (!node)
   return -ENOMEM;
 @@ -770,6 +773,7 @@ static void connect(struct backend_info *be)
   }
  
   xen_net_read_rate(dev, credit_bytes, credit_usec);
 + xen_unregister_watchers(be-vif);
   xen_register_watchers(dev, be-vif);
   read_xenbus_vif_flags(be);
  
 -- 
 1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe netdev in

Re: [PATCH] xen-netback:Make the function xenvif_schedulable have a return type of bool

[Wei Liu]

On Tue, Jun 16, 2015 at 11:03:30PM -0400, Nicholas Krause wrote:
 This makes the function xenvif_sechedulable have a return type of
 bool now due to this particular function's return statement only
 ever evaluating to have a value of one or zero.
 
 Signed-off-by: Nicholas Krause xerofo...@gmail.com

Acked-by: Wei Liu wei.l...@citrix.com

 ---
  drivers/net/xen-netback/common.h| 2 +-
  drivers/net/xen-netback/interface.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/net/xen-netback/common.h 
 b/drivers/net/xen-netback/common.h
 index 8a495b3..c02cafb 100644
 --- a/drivers/net/xen-netback/common.h
 +++ b/drivers/net/xen-netback/common.h
 @@ -280,7 +280,7 @@ void xenvif_free(struct xenvif *vif);
  int xenvif_xenbus_init(void);
  void xenvif_xenbus_fini(void);
  
 -int xenvif_schedulable(struct xenvif *vif);
 +bool xenvif_schedulable(struct xenvif *vif);
  
  int xenvif_queue_stopped(struct xenvif_queue *queue);
  void xenvif_wake_queue(struct xenvif_queue *queue);
 diff --git a/drivers/net/xen-netback/interface.c 
 b/drivers/net/xen-netback/interface.c
 index 1a83e19..b5fcb52 100644
 --- a/drivers/net/xen-netback/interface.c
 +++ b/drivers/net/xen-netback/interface.c
 @@ -63,7 +63,7 @@ void xenvif_skb_zerocopy_complete(struct xenvif_queue 
 *queue)
   atomic_dec(queue-inflight_packets);
  }
  
 -int xenvif_schedulable(struct xenvif *vif)
 +bool xenvif_schedulable(struct xenvif *vif)
  {
   return netif_running(vif-dev) 
   test_bit(VIF_STATUS_CONNECTED, vif-status) 
 -- 
 2.1.4
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 03/13] xen-netback: implement TX persistent grants

[Wei Liu]

On Wed, Jun 03, 2015 at 05:07:59PM +, Joao Martins wrote:
[...]
  
  How much harder would it be to ref-count inflight grants? Would that
  simplify or perplex things? I'm just asking, not suggesting you should
  choose ref-counting over current scheme.
  
  In principle I favour simple code path over optimisation for every
  possible corner case.
 
 ref-counting the persistent grants would mean eliminating the check for
 EBUSY on xenvif_pgrant_new, but though it isn’t that much of a simplification.
 

Right.

 What would simplify a lot is if I grant map when we don’t get a persistent_gnt
 in xenvif_pgrant_new() and add it to the tree there instead of doing on 
 xenvif_tx_check_gop.
 Since this happens only once for persistent grants (and up to ring size), I 
 believe it
 wouldn't hurt performance.
 

Yeah. Mapping page inside xenvif_tx_check_gop doesn't sound nice.

 This way we would remove a lot of the checks in xenvif_tx_check_gop and
 hopefully leaving those parts (almost) intact mainly to be used for grant
 map/unmap case. The reason I didn’t do it because I wanted to reuse the
 grant map code and thought that preference was given towards batching the
 grant maps. But it looks that it definitely makes things more complicated
 and adds more corner cases.
 
 The same goes for the RX case where this change would remove a lot of the code
 for adding the grant maps (thus sharing a lot from the TX part) besides 
 removing the
 mixed initial grant copy + map. What do you think?
 

I couldn't really comment until I see the code. But in principle I think
this is a step towards the right direction.

Wei.

 Joao
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next] tcp: double default TSQ output bytes limit

[Wei Liu]

Xen virtual network driver has higher latency than a physical NIC.
Having only 128K as limit for TSQ introduced 30% regression in guest
throughput.

This patch raises the limit to 256K. This reduces the regression to 8%.
This buys us more time to work out a proper solution in the long run.

Signed-off-by: Wei Liu wei.l...@citrix.com
Cc: David Miller da...@davemloft.net
Cc: Eric Dumazet eric.duma...@gmail.com
---
 net/ipv4/tcp_output.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 190538a..eeb59be 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -50,8 +50,8 @@ int sysctl_tcp_retrans_collapse __read_mostly = 1;
  */
 int sysctl_tcp_workaround_signed_windows __read_mostly = 0;
 
-/* Default TSQ limit of two TSO segments */
-int sysctl_tcp_limit_output_bytes __read_mostly = 131072;
+/* Default TSQ limit of four TSO segments */
+int sysctl_tcp_limit_output_bytes __read_mostly = 262144;
 
 /* This limits the percentage of the congestion window which we
  * will allow a single TSO frame to consume.  Building TSO frames
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] tcp: refine TSO autosizing causes performance regression on Xen

[Wei Liu]

Hi Eric

Sorry for coming late to the discussion.

On Thu, Apr 16, 2015 at 05:42:16AM -0700, Eric Dumazet wrote:
 On Thu, 2015-04-16 at 11:01 +0100, George Dunlap wrote:
 
  He suggested that after he'd been prodded by 4 more e-mails in which two
  of us guessed what he was trying to get at.  That's what I was
  complaining about.
 
 My big complain is that I suggested to test to double the sysctl, which
 gave good results.
 

Do I understand correctly that it's acceptable to you to double the size
of the buffer? If so I will send a patch to do that.

Wei.

 Then you provided a patch using a 8x factor. How does that sound ?
 
 Next time I ask a raise, I should try a 8x factor as well, who knows,
 it might be accepted.
 
 
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 03/13] xen-netback: implement TX persistent grants

[Wei Liu]

On Fri, May 22, 2015 at 10:24:39AM +, Joao Martins wrote:
 
 On 19 May 2015, at 17:23, Wei Liu wei.l...@citrix.com wrote:
  On Tue, May 12, 2015 at 07:18:27PM +0200, Joao Martins wrote:
  Introduces persistent grants for TX path which follows similar code path
  as the grant mapping.
  
  It starts by checking if there's a persistent grant available for header
  and frags grefs and if so setting it in tx_pgrants. If no persistent grant
  is found in the tree for the header it will resort to grant copy (but
  preparing the map ops and add them laster). For the frags it will use the
  ^
  later
  
  tree page pool, and in case of no pages it fallbacks to grant map/unmap
  using mmap_pages. When skb destructor callback gets called we release the
  slot and persistent grant within the callback to avoid waking up the
  dealloc thread. As long as there are no unmaps to done the dealloc thread
  will remain inactive.
  
  
  This scheme looks complicated. Can we just only use one
  scheme at a time? What's the rationale for using this combined scheme?
  Maybe you're thinking about using a max_grants  ring_size to save
  memory?
 
 Yes, my purpose was to allow a max_grants  ring_size to save amount of
 memory mapped. I did a bulk transfer test with iperf and the max amount of
 grants in tree was 160 TX gnts, without affecting the max performance;
 tough using pktgen fills the tree completely.
 The second reason is to handle the case for a (malicious?) frontend providing
 more grefs than the max allowed in which I would fallback to grant map/unmap.
 

This is indeed a valid concern. The only method is to expires oldest
grant when that happens -- but this is just complexity in another place,
not really simplifying anything.

  
  Only skim the patch. I will do detailed reviews after we're sure this is
  the right way to go.
  
[...]
  
  Under what circumstance can we retrieve a already in use persistent
  grant? You seem to suggest this is a bug in RX case.
 
 A guest could share try to share the same mapped page in multiple frags,
 in which case I fallback to map/unmap. I think this is a limitation in
 the way we manage the persistent gnts where we can only have a single
 reference of a persistent grant inflight.
 

How much harder would it be to ref-count inflight grants? Would that
simplify or perplex things? I'm just asking, not suggesting you should
choose ref-counting over current scheme.

In principle I favour simple code path over optimisation for every
possible corner case.

Wei.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html