From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id D2F22A0613 for ; Tue, 24 Sep 2019 22:37:56 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 18CD52BE1; Tue, 24 Sep 2019 22:37:56 +0200 (CEST) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 5DC012BCE for ; Tue, 24 Sep 2019 22:37:54 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BB30980F91; Tue, 24 Sep 2019 20:37:53 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.12]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4F7585B6A5; Tue, 24 Sep 2019 20:37:53 +0000 (UTC) From: Aaron Conole To: Igor Ryzhov Cc: dev@dpdk.org References: <20190924192548.17291-1-iryzhov@nfware.com> <20190924193312.17381-1-iryzhov@nfware.com> Date: Tue, 24 Sep 2019 16:37:52 -0400 In-Reply-To: <20190924193312.17381-1-iryzhov@nfware.com> (Igor Ryzhov's message of "Tue, 24 Sep 2019 22:33:12 +0300") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 24 Sep 2019 20:37:53 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH v2] kni: rework rte_kni_update_link using ioctl X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Igor Ryzhov 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 > --- > 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