Re: [dpdk-dev] [PATCH v2] kni: rework rte_kni_update_link using ioctl

[Aaron Conole <aconole@redhat.com>]

Igor Ryzhov <iryzhov@nfware.com> writes:

> Current implementation doesn't allow us to update KNI carrier if the
> interface is not yet UP in kernel. It means that we can't use it in the
> same thread which is processing rte_kni_ops.config_network_if, which is
> very convenient, because it allows us to have correct carrier status
> of the interface right after we enabled it and we don't have to use any
> additional thread to track link status.
>
> Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
> ---
> v2: fix checkpatch warnings
>
>  kernel/linux/kni/compat.h                     |  4 --
>  kernel/linux/kni/kni_misc.c                   | 42 +++++++++++++++++++
>  kernel/linux/kni/kni_net.c                    | 15 -------
>  .../linux/eal/include/rte_kni_common.h        |  6 +++
>  lib/librte_kni/rte_kni.c                      | 28 +++----------
>  lib/librte_kni/rte_kni.h                      |  3 +-
>  6 files changed, 55 insertions(+), 43 deletions(-)
>
> diff --git a/kernel/linux/kni/compat.h b/kernel/linux/kni/compat.h
> index fe0ee55e7..e0a491bcd 100644
> --- a/kernel/linux/kni/compat.h
> +++ b/kernel/linux/kni/compat.h
> @@ -61,10 +61,6 @@
>  #define kni_sock_map_fd(s) sock_map_fd(s, 0)
>  #endif
>  
> -#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 9, 0)
> -#define HAVE_CHANGE_CARRIER_CB
> -#endif
> -
>  #if LINUX_VERSION_CODE < KERNEL_VERSION(3, 14, 0)
>  #define ether_addr_copy(dst, src) memcpy(dst, src, ETH_ALEN)
>  #endif
> diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
> index aeb275329..9bcba68c8 100644
> --- a/kernel/linux/kni/kni_misc.c
> +++ b/kernel/linux/kni/kni_misc.c
> @@ -462,6 +462,45 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
>  	return ret;
>  }
>  
> +static int
> +kni_ioctl_link(struct net *net, uint32_t ioctl_num,
> +		unsigned long ioctl_param)
> +{
> +	struct kni_net *knet = net_generic(net, kni_net_id);
> +	int ret = -EINVAL;
> +	struct kni_dev *dev, *n;
> +	struct rte_kni_link_info link_info;
> +	struct net_device *netdev;
> +
> +	if (_IOC_SIZE(ioctl_num) > sizeof(link_info))
> +		return -EINVAL;
> +
> +	if (copy_from_user(&link_info, (void *)ioctl_param, sizeof(link_info)))
> +		return -EFAULT;
> +
> +	if (strlen(link_info.name) == 0)
> +		return -EINVAL;
> +
> +	down_read(&knet->kni_list_lock);
> +	list_for_each_entry_safe(dev, n, &knet->kni_list_head, list) {
> +		if (strncmp(dev->name, link_info.name, RTE_KNI_NAMESIZE) != 0)
> +			continue;
> +
> +		netdev = dev->net_dev;
> +
> +		if (link_info.linkup)
> +			netif_carrier_on(netdev);
> +		else
> +			netif_carrier_off(netdev);
> +
> +		ret = 0;
> +		break;
> +	}
> +	up_read(&knet->kni_list_lock);
> +
> +	return ret;
> +}
> +
>  static int
>  kni_ioctl(struct inode *inode, uint32_t ioctl_num, unsigned long ioctl_param)
>  {
> @@ -483,6 +522,9 @@ kni_ioctl(struct inode *inode, uint32_t ioctl_num, unsigned long ioctl_param)
>  	case _IOC_NR(RTE_KNI_IOCTL_RELEASE):
>  		ret = kni_ioctl_release(net, ioctl_num, ioctl_param);
>  		break;
> +	case _IOC_NR(RTE_KNI_IOCTL_LINK):
> +		ret = kni_ioctl_link(net, ioctl_num, ioctl_param);
> +		break;
>  	default:
>  		pr_debug("IOCTL default\n");
>  		break;
> diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
> index 7bd3a9f1e..cd852eea3 100644
> --- a/kernel/linux/kni/kni_net.c
> +++ b/kernel/linux/kni/kni_net.c
> @@ -706,18 +706,6 @@ kni_net_set_mac(struct net_device *netdev, void *p)
>  	return (ret == 0 ? req.result : ret);
>  }
>  
> -#ifdef HAVE_CHANGE_CARRIER_CB
> -static int
> -kni_net_change_carrier(struct net_device *dev, bool new_carrier)
> -{
> -	if (new_carrier)
> -		netif_carrier_on(dev);
> -	else
> -		netif_carrier_off(dev);
> -	return 0;
> -}
> -#endif
> -
>  static const struct header_ops kni_net_header_ops = {
>  	.create  = kni_net_header,
>  	.parse   = eth_header_parse,
> @@ -736,9 +724,6 @@ static const struct net_device_ops kni_net_netdev_ops = {
>  	.ndo_change_mtu = kni_net_change_mtu,
>  	.ndo_tx_timeout = kni_net_tx_timeout,
>  	.ndo_set_mac_address = kni_net_set_mac,
> -#ifdef HAVE_CHANGE_CARRIER_CB
> -	.ndo_change_carrier = kni_net_change_carrier,
> -#endif
>  };
>  
>  static void kni_get_drvinfo(struct net_device *dev,
> diff --git a/lib/librte_eal/linux/eal/include/rte_kni_common.h b/lib/librte_eal/linux/eal/include/rte_kni_common.h
> index 70992d835..07a10dd93 100644
> --- a/lib/librte_eal/linux/eal/include/rte_kni_common.h
> +++ b/lib/librte_eal/linux/eal/include/rte_kni_common.h
> @@ -125,10 +125,16 @@ struct rte_kni_device_info {
>  	uint8_t mac_addr[6];
>  };
>  
> +struct rte_kni_link_info {
> +	char name[RTE_KNI_NAMESIZE];
> +	unsigned int linkup;
> +};
> +
>  #define KNI_DEVICE "kni"
>  
>  #define RTE_KNI_IOCTL_TEST    _IOWR(0, 1, int)
>  #define RTE_KNI_IOCTL_CREATE  _IOWR(0, 2, struct rte_kni_device_info)
>  #define RTE_KNI_IOCTL_RELEASE _IOWR(0, 3, struct rte_kni_device_info)
> +#define RTE_KNI_IOCTL_LINK    _IOWR(0, 4, struct rte_kni_link_info)
>  
>  #endif /* _RTE_KNI_COMMON_H_ */
> diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
> index 521db27c4..bdea03d5c 100644
> --- a/lib/librte_kni/rte_kni.c
> +++ b/lib/librte_kni/rte_kni.c
> @@ -738,36 +738,20 @@ rte_kni_update_link(struct rte_kni *kni, unsigned int linkup)

You'vbe dropped a lot of code from here, but no the corresponding
variables.  This will trigger lots of warnings.  Please build with all
warnings and fix them.

>  	const char *new_carrier;
>  	int old_linkup;
>  	int fd, ret;
> +	struct rte_kni_link_info link_info;
>  
>  	if (kni == NULL)
>  		return -1;
>  
> -	snprintf(path, sizeof(path), "/sys/devices/virtual/net/%s/carrier",
> -		kni->name);
> +	snprintf(link_info.name, RTE_KNI_NAMESIZE, "%s", kni->name);
> +	link_info.linkup = linkup;
>  
> -	fd = open(path, O_RDWR);
> -	if (fd == -1) {
> -		RTE_LOG(ERR, KNI, "Failed to open file: %s.\n", path);
> +	if (ioctl(kni_fd, RTE_KNI_IOCTL_LINK, &link_info) < 0) {
> +		RTE_LOG(ERR, KNI, "Fail to update KNI link\n");
>  		return -1;
>  	}
>  
> -	ret = read(fd, old_carrier, 2);
> -	if (ret < 1) {
> -		close(fd);
> -		return -1;
> -	}
> -	old_linkup = (old_carrier[0] == '1');
> -
> -	new_carrier = linkup ? "1" : "0";
> -	ret = write(fd, new_carrier, 1);
> -	if (ret < 1) {
> -		RTE_LOG(ERR, KNI, "Failed to write file: %s.\n", path);
> -		close(fd);
> -		return -1;
> -	}
> -
> -	close(fd);
> -	return old_linkup;
> +	return 0;
>  }
>  
>  void
> diff --git a/lib/librte_kni/rte_kni.h b/lib/librte_kni/rte_kni.h
> index b22446fa7..7e1cce1e0 100644
> --- a/lib/librte_kni/rte_kni.h
> +++ b/lib/librte_kni/rte_kni.h
> @@ -247,9 +247,8 @@ int rte_kni_unregister_handlers(struct rte_kni *kni);
>   *  > 0 for linkup.
>   *
>   * @return
> + *  On success: 0
>   *  On failure: -1
> - *  Previous link state == linkdown: 0
> - *  Previous link state == linkup: 1
>   */
>  __rte_experimental
>  int