[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 20/04/18 12:47, Stanislav Kinsburskii wrote:
> This patch adds wrapper helpers around generic Xen fault inject facility.
> The major reason is to keep all the module fault injection directories
> in a dedicated subdirectory instead of Xen fault inject root.
> 
> IOW, when using these helpers, per-device and named by device name fault
> injection control directories will appear under the following directory:
> - /sys/kernel/debug/xen/fault_inject/xen-netback/
> instead of:
> - /sys/kernel/debug/xen/fault_inject/
> 
> Signed-off-by: Stanislav Kinsburskii <staskins@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> CC: "David S. Miller" <davem@xxxxxxxxxxxxx>
> CC: Matteo Croce <mcroce@xxxxxxxxxx>
> CC: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
> CC: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> CC: Gerard Garcia <ggarcia@xxxxxxxxxxxx>
> CC: David Ahern <dsa@xxxxxxxxxxxxxxxxxxx>
> CC: Juergen Gross <jgross@xxxxxxxx>
> CC: Amir Levy <amir.jer.levy@xxxxxxxxx>
> CC: Jakub Kicinski <jakub.kicinski@xxxxxxxxxxxxx>
> CC: linux-kernel@xxxxxxxxxxxxxxx
> CC: netdev@xxxxxxxxxxxxxxx
> CC: xen-devel@xxxxxxxxxxxxxxxxxxxx
> CC: Stanislav Kinsburskii <staskins@xxxxxxxxxx>
> CC: David Woodhouse <dwmw@xxxxxxxxxxxx>
> ---
>  drivers/net/Kconfig                  |    8 ++
>  drivers/net/xen-netback/Makefile     |    1 
>  drivers/net/xen-netback/common.h     |    3 +
>  drivers/net/xen-netback/netback.c    |    3 +
>  drivers/net/xen-netback/netback_fi.c |  119 
> ++++++++++++++++++++++++++++++++++
>  drivers/net/xen-netback/netback_fi.h |   35 ++++++++++
>  drivers/net/xen-netback/xenbus.c     |    6 ++
>  7 files changed, 175 insertions(+)
>  create mode 100644 drivers/net/xen-netback/netback_fi.c
>  create mode 100644 drivers/net/xen-netback/netback_fi.h
> 
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 8918466..5cc9acd 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -465,6 +465,14 @@ config XEN_NETDEV_BACKEND
>         compile this driver as a module, chose M here: the module
>         will be called xen-netback.
>  
> +config XEN_NETDEV_BACKEND_FAULT_INJECTION
> +       bool "Xen net-device backend driver fault injection"
> +       depends on XEN_NETDEV_BACKEND
> +       depends on XEN_FAULT_INJECTION
> +       default n
> +       help
> +         Allow to inject errors to Xen backend network driver
> +
>  config VMXNET3
>       tristate "VMware VMXNET3 ethernet driver"
>       depends on PCI && INET
> diff --git a/drivers/net/xen-netback/Makefile 
> b/drivers/net/xen-netback/Makefile
> index d49798a..28abcdc 100644
> --- a/drivers/net/xen-netback/Makefile
> +++ b/drivers/net/xen-netback/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_XEN_NETDEV_BACKEND) := xen-netback.o
>  
>  xen-netback-y := netback.o xenbus.o interface.o hash.o rx.o
> +xen-netback-$(CONFIG_XEN_NETDEV_BACKEND_FAULT_INJECTION) += netback_fi.o
> diff --git a/drivers/net/xen-netback/common.h 
> b/drivers/net/xen-netback/common.h
> index a46a1e9..30d676d 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -286,6 +286,9 @@ struct xenvif {
>  
>  #ifdef CONFIG_DEBUG_FS
>       struct dentry *xenvif_dbg_root;
> +#ifdef CONFIG_XEN_NETDEV_BACKEND_FAULT_INJECTION
> +     void *fi_info;
> +#endif
>  #endif
>  
>       struct xen_netif_ctrl_back_ring ctrl;
> diff --git a/drivers/net/xen-netback/netback.c 
> b/drivers/net/xen-netback/netback.c
> index a27daa2..ecc416e 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -33,6 +33,7 @@
>   */
>  
>  #include "common.h"
> +#include "netback_fi.h"
>  
>  #include <linux/kthread.h>
>  #include <linux/if_vlan.h>
> @@ -1649,6 +1650,7 @@ static int __init netback_init(void)
>                       PTR_ERR(xen_netback_dbg_root));
>  #endif /* CONFIG_DEBUG_FS */
>  
> +     (void) xen_netbk_fi_init();

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

>       return 0;
>  
>  failed_init:
> @@ -1659,6 +1661,7 @@ module_init(netback_init);
>  
>  static void __exit netback_fini(void)
>  {
> +     xen_netbk_fi_fini();
>  #ifdef CONFIG_DEBUG_FS
>       if (!IS_ERR_OR_NULL(xen_netback_dbg_root))
>               debugfs_remove_recursive(xen_netback_dbg_root);
> diff --git a/drivers/net/xen-netback/netback_fi.c 
> b/drivers/net/xen-netback/netback_fi.c
> new file mode 100644
> index 0000000..47541d0
> --- /dev/null
> +++ b/drivers/net/xen-netback/netback_fi.c
> @@ -0,0 +1,119 @@
> +/*
> + * Fault injection interface for Xen backend network driver
> + *
> + * Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation; or, when distributed
> + * separately from the Linux kernel or incorporated into other
> + * software packages, subject to the following license:

SPDX again.

> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this source file (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use, copy, modify,
> + * merge, publish, distribute, sublicense, and/or sell copies of the 
> Software,
> + * and to permit persons to whom the Software is furnished to do so, subject 
> to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
> THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "common.h"
> +
> +#include <linux/debugfs.h>
> +
> +#include <xen/fault_inject.h>
> +#include "netback_fi.h"
> +
> +static struct dentry *vif_fi_dir;
> +
> +static const char *xenvif_fi_names[] = {
> +};
> +
> +struct xenvif_fi {
> +     struct dentry *dir;
> +     struct xen_fi *faults[XENVIF_FI_MAX];
> +};
> +
> +int xen_netbk_fi_init(void)
> +{
> +     vif_fi_dir = xen_fi_dir_create("xen-netback");
> +     if (!vif_fi_dir)
> +             return -ENOMEM;
> +     return 0;
> +}
> +
> +void xen_netbk_fi_fini(void)
> +{
> +     debugfs_remove_recursive(vif_fi_dir);
> +}
> +
> +void xenvif_fi_fini(struct xenvif *vif)
> +{
> +     struct xenvif_fi *vfi = vif->fi_info;
> +     int fi;
> +
> +     if (!vif->fi_info)
> +             return;
> +
> +     vif->fi_info = NULL;
> +
> +     for (fi = 0; fi < XENVIF_FI_MAX; fi++)
> +             xen_fi_del(vfi->faults[fi]);
> +     debugfs_remove_recursive(vfi->dir);
> +     kfree(vfi);
> +}
> +
> +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]);

How does this work? xenvif_fi_names[] is an empty array and this is the
only reference to it. Who is allocating the memory for that array?

> +             if (!vfi->faults[fi])
> +                     goto err_fault;
> +     }
> +
> +     vif->fi_info = vfi;
> +     return 0;
> +
> +err_fault:
> +     for (; fi > 0; fi--)
> +             xen_fi_del(vfi->faults[fi]);

What about vfi->faults[0] ?

> +     debugfs_remove_recursive(vfi->dir);
> +err_dir:
> +     kfree(vfi);
> +     return err;
> +}
> +
> +bool xenvif_should_fail(struct xenvif *vif, xenvif_fi_t type)
> +{
> +     struct xenvif_fi *vfi = vif->fi_info;
> +
> +     return xen_should_fail(vfi->faults[type]);
> +}
> diff --git a/drivers/net/xen-netback/netback_fi.h 
> b/drivers/net/xen-netback/netback_fi.h
> new file mode 100644
> index 0000000..895c6a6
> --- /dev/null
> +++ b/drivers/net/xen-netback/netback_fi.h
> @@ -0,0 +1,35 @@
> +#ifndef _XEN_NETBACK_FI_H
> +#define _XEN_NETBACK_FI_H
> +
> +struct xen_fi;

Why?

> +
> +typedef enum {
> +     XENVIF_FI_MAX
> +} xenvif_fi_t;

It would have helped if you had added some users of the stuff you are
adding here. This enum just looks weird this way.

> +
> +#ifdef CONFIG_XEN_NETDEV_BACKEND_FAULT_INJECTION
> +
> +int xen_netbk_fi_init(void);
> +void xen_netbk_fi_fini(void);
> +
> +void xenvif_fi_fini(struct xenvif *vif);
> +int xenvif_fi_init(struct xenvif *vif);
> +
> +bool xenvif_should_fail(struct xenvif *vif, xenvif_fi_t type);
> +
> +#else
> +
> +static inline int xen_netbk_fi_init(void) { return 0; }
> +static inline void xen_netbk_fi_fini(void) { }
> +
> +static inline void xenvif_fi_fini(struct xenvif *vif) { }
> +static inline int xenvif_fi_init(struct xenvif *vif) { return 0; }
> +
> +static inline bool xenvif_should_fail(struct xenvif *vif, xenvif_fi_t type)
> +{
> +     return false;
> +}
> +
> +#endif /* CONFIG_XEN_NETDEV_BACKEND_FAULT_INJECTION */
> +
> +#endif /* _XEN_NETBACK_FI_H */
> diff --git a/drivers/net/xen-netback/xenbus.c 
> b/drivers/net/xen-netback/xenbus.c
> index e1aef25..c775ee0 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -21,6 +21,7 @@
>  #include "common.h"
>  #include <linux/vmalloc.h>
>  #include <linux/rtnetlink.h>
> +#include "netback_fi.h"
>  
>  struct backend_info {
>       struct xenbus_device *dev;
> @@ -502,6 +503,7 @@ static void backend_disconnect(struct backend_info *be)
>  #ifdef CONFIG_DEBUG_FS
>               xenvif_debugfs_delif(vif);
>  #endif /* CONFIG_DEBUG_FS */
> +             xenvif_fi_fini(vif);
>               xenvif_disconnect_data(vif);
>  
>               /* At this point some of the handlers may still be active
> @@ -1024,6 +1026,10 @@ static void connect(struct backend_info *be)
>               }
>       }
>  
> +     err = xenvif_fi_init(be->vif);
> +     if (err)
> +             goto err;
> +
>  #ifdef CONFIG_DEBUG_FS
>       xenvif_debugfs_addif(be->vif);
>  #endif /* CONFIG_DEBUG_FS */
>

Without any user of that infrastructure I really can't say whether I
want this.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.