From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id C84F9A0C47; Tue, 6 Jul 2021 11:14:44 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 780F040688; Tue, 6 Jul 2021 11:14:44 +0200 (CEST) Received: from mail-ed1-f50.google.com (mail-ed1-f50.google.com [209.85.208.50]) by mails.dpdk.org (Postfix) with ESMTP id 8C8EF4067E for ; Tue, 6 Jul 2021 11:14:42 +0200 (CEST) Received: by mail-ed1-f50.google.com with SMTP id h2so27081821edt.3 for ; Tue, 06 Jul 2021 02:14:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nfware.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=0bAxPasjPyUmFTBRVu9aF9u3TQXSmFGmXeDVlRp2PAU=; b=D5iwivLEy/uFCchqpFYhD+/O2rt1NFeWtMI3qHQmiRzIOdpcJYlIZWuWDrt+NjK5pa AeboG2m5o5556M96XUSWw+aRlAoH3HKF9kiWoh5l64oKfzPKF9UGJqa/XkPbwhpA/ZgJ 0PE9gINqOrvD/nqK9WrMeVed3v2V1SyMcU5PQiGmCWzHe9yFO5r3KfqcapFyS0j/1Dk2 9bftMSrNxzqkp8BimuoTtzThHUEmF373/2fjUIQ1BM9jrccA5kXtsUkuKhfVaCtO+Jsm Yvv/vOeMB/WikOrs1vW7dTWSGXZg11jTe36eQffOR5QF9LRaiuUpCctPB+41GUZwruir cbfg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=0bAxPasjPyUmFTBRVu9aF9u3TQXSmFGmXeDVlRp2PAU=; b=j0kQ+UBjXsB7M3iYzWF9VjrtM1uDvXYvu+7DT0lYJG6dVnZPzD2+456i92GRcNCBuH SyRLIWeKHLAVhIlWMTHrivtlgUGYo8WaXyjr2ij28MulndEqPrT9CqJ88MlWdpDkfcTt XiB8yq20vuWoSJ82VW7RiyyGMCKRO/4e/Q5dYLi1jZRGJts7OMkLCFhViJcdcYNHMEKq VlSULSnxZvysfxiWB9RMIlog6nVNf4emk4Yh4vh90Uh16Ze1JIVRsalk0dsXNltu+BrU /1uJoS6cGA1EkZFNH3mH1yVMsA/p3j9IqklBl2P4jyixXlO1s9+5gfTDwJkRtoGkdknw TaSQ== X-Gm-Message-State: AOAM531UhJ00/fjwP1ARNhLCiCmZVmSoIbGNbim2pmaqkuOwde1TCfr1 NuwKMLgb0YCpAbqqs2qE8UuKrvwBFswn4givHFeGBg== X-Google-Smtp-Source: ABdhPJwWLhGe3P8sq1l+eoY9ki7b9ymVvqL7uyJtAOzeR+5CYHgWCORTaq0xUQ7rB7eeK0j3VlL3d0ZQEw5Oyu0g54Y= X-Received: by 2002:aa7:d94c:: with SMTP id l12mr11589262eds.102.1625562882248; Tue, 06 Jul 2021 02:14:42 -0700 (PDT) MIME-Version: 1.0 References: <20190924193312.17381-1-iryzhov@nfware.com> <20210704160610.62682-1-iryzhov@nfware.com> <415de2ea-8ea0-18c3-af10-280a9f4fe8da@intel.com> In-Reply-To: <415de2ea-8ea0-18c3-af10-280a9f4fe8da@intel.com> From: Igor Ryzhov Date: Tue, 6 Jul 2021 12:14:31 +0300 Message-ID: To: Ferruh Yigit Cc: dev , Dan Gora Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [dpdk-dev] [PATCH v4 1/2] kni: rework rte_kni_update_link using ioctl X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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" Hi Ferruh, Thanks for the review. My comments inline. I'll send a new version later this week. On Mon, Jul 5, 2021 at 2:58 PM Ferruh Yigit wrote: > On 7/4/2021 6:06 PM, Igor Ryzhov wrote: > > 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. > > > > Propagating speed/duplex/autoneg to the kernel module also allows us to > > implement ethtool_ops.get_link_ksettings callback. > > > > Suggested-by: Dan Gora > > Signed-off-by: Igor Ryzhov > > --- > > app/test/test_kni.c | 32 ++++++++++--------------- > > examples/kni/main.c | 2 +- > > kernel/linux/kni/compat.h | 4 ---- > > kernel/linux/kni/kni_dev.h | 5 ++++ > > kernel/linux/kni/kni_misc.c | 47 +++++++++++++++++++++++++++++++++++++ > > kernel/linux/kni/kni_net.c | 15 ------------ > > lib/kni/rte_kni.c | 38 ++++++++---------------------- > > lib/kni/rte_kni.h | 10 ++++---- > > lib/kni/rte_kni_common.h | 9 +++++++ > > 9 files changed, 89 insertions(+), 73 deletions(-) > > > > diff --git a/app/test/test_kni.c b/app/test/test_kni.c > > index 96733554b6c4..f9300552b3f6 100644 > > --- a/app/test/test_kni.c > > +++ b/app/test/test_kni.c > > @@ -125,6 +125,7 @@ kni_change_mtu(uint16_t port_id, unsigned int > new_mtu) > > static int > > test_kni_link_change(void) > > { > > + struct rte_eth_link link; > > int ret; > > int pid; > > > > @@ -135,42 +136,35 @@ test_kni_link_change(void) > > } > > > > if (pid == 0) { > > + link.link_speed = ETH_SPEED_NUM_10G; > > + link.link_duplex = ETH_LINK_FULL_DUPLEX; > > + link.link_autoneg = ETH_LINK_AUTONEG; > > + > > printf("Starting KNI Link status change tests.\n"); > > if (system(IFCONFIG TEST_KNI_PORT" up") == -1) { > > ret = -1; > > goto error; > > } > > > > - ret = rte_kni_update_link(test_kni_ctx, 1); > > + link.link_status = ETH_LINK_UP; > > + ret = rte_kni_update_link(test_kni_ctx, &link); > > if (ret < 0) { > > printf("Failed to change link state to Up > ret=%d.\n", > > ret); > > goto error; > > } > > rte_delay_ms(1000); > > - printf("KNI: Set LINKUP, previous state=%d\n", ret); > > - > > - ret = rte_kni_update_link(test_kni_ctx, 0); > > - if (ret != 1) { > > - printf( > > - "Failed! Previous link state should be 1, returned %d.\n", > > - ret); > > - goto error; > > - } > > - rte_delay_ms(1000); > > - printf("KNI: Set LINKDOWN, previous state=%d\n", ret); > > + printf("KNI: Set LINKUP\n"); > > > > - ret = rte_kni_update_link(test_kni_ctx, 1); > > - if (ret != 0) { > > - printf( > > - "Failed! Previous link state should be 0, returned %d.\n", > > + link.link_status = ETH_LINK_DOWN; > > + ret = rte_kni_update_link(test_kni_ctx, &link); > > + if (ret < 0) { > > + printf("Failed to change link state to Down > ret=%d.\n", > > ret); > > goto error; > > } > > Is there a way to verify the link status of the KNI interface, to double > check > setting link status succedded? > Yes, we can still read from the carrier file to check the current status. I'll add this to the test. > > > - printf("KNI: Set LINKUP, previous state=%d\n", ret); > > - > > - ret = 0; > > rte_delay_ms(1000); > > + printf("KNI: Set LINKDOWN\n"); > > > > error: > > if (system(IFCONFIG TEST_KNI_PORT" down") == -1) > > diff --git a/examples/kni/main.c b/examples/kni/main.c > > index beabb3c848aa..5833037cf1c9 100644 > > --- a/examples/kni/main.c > > +++ b/examples/kni/main.c > > @@ -755,7 +755,7 @@ monitor_all_ports_link_status(void *arg) > > } > > for (i = 0; i < p[portid]->nb_kni; i++) { > > prev = > rte_kni_update_link(p[portid]->kni[i], > > - link.link_status); > > + &link); > > log_link_state(p[portid]->kni[i], prev, > &link); > > } > > } > > diff --git a/kernel/linux/kni/compat.h b/kernel/linux/kni/compat.h > > index 5f65640d5ed2..995d109c275a 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_dev.h b/kernel/linux/kni/kni_dev.h > > index c15da311ba25..969108cc30f8 100644 > > --- a/kernel/linux/kni/kni_dev.h > > +++ b/kernel/linux/kni/kni_dev.h > > @@ -88,6 +88,11 @@ struct kni_dev { > > void *alloc_va[MBUF_BURST_SZ]; > > > > struct task_struct *usr_tsk; > > + > > + /* correct when netif_carrier_ok */ > > + uint32_t speed; > > + uint8_t duplex; > > + uint8_t autoneg; > > }; > > > > #ifdef HAVE_IOVA_TO_KVA_MAPPING_SUPPORT > > diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c > > index 2b464c438113..30ee69661935 100644 > > --- a/kernel/linux/kni/kni_misc.c > > +++ b/kernel/linux/kni/kni_misc.c > > @@ -481,6 +481,50 @@ 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.status) { > > + netif_carrier_on(netdev); > > + > > + dev->speed = link_info.speed; > > + dev->duplex = link_info.duplex; > > + dev->autoneg = link_info.autoneg; > > + } 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) > > { > > @@ -502,6 +546,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 611719b5ee27..99da8d37dd6b 100644 > > --- a/kernel/linux/kni/kni_net.c > > +++ b/kernel/linux/kni/kni_net.c > > @@ -778,18 +778,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, > > @@ -808,9 +796,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 > > What do you think keeping the 'carrier' interface? Even the API not using > it, it > can be useful, what do you think? > We can keep it, but if people continue to write to the carrier file instead of using the API, then speed/duplex/autoneg values will have unspecified values. We can reflect this in the docs. > > > }; > > > > static void kni_get_drvinfo(struct net_device *dev, > > diff --git a/lib/kni/rte_kni.c b/lib/kni/rte_kni.c > > index eb24b0d0ae4e..0a7b562abf11 100644 > > --- a/lib/kni/rte_kni.c > > +++ b/lib/kni/rte_kni.c > > @@ -784,43 +784,25 @@ rte_kni_unregister_handlers(struct rte_kni *kni) > > } > > > > int > > -rte_kni_update_link(struct rte_kni *kni, unsigned int linkup) > > +rte_kni_update_link(struct rte_kni *kni, struct rte_eth_link *link) > > { > > - char path[64]; > > - char old_carrier[2]; > > - 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.speed = link->link_speed; > > + link_info.duplex = link->link_duplex; > > + link_info.autoneg = link->link_autoneg; > > + link_info.status = link->link_status; > > > > - 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; > > } > > Can you please add new API to KNI PMD too? To set the link up when port > started. > I'll check KNI PMD and update. > > > > > void > > diff --git a/lib/kni/rte_kni.h b/lib/kni/rte_kni.h > > index b0eaf4610416..ab5990fd210d 100644 > > --- a/lib/kni/rte_kni.h > > +++ b/lib/kni/rte_kni.h > > @@ -21,6 +21,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > > > @@ -245,18 +246,15 @@ int rte_kni_unregister_handlers(struct rte_kni > *kni); > > * @param kni > > * pointer to struct rte_kni. > > * @param linkup > > - * New link state: > > - * 0 for linkdown. > > - * > 0 for linkup. > > + * new link state, speed, duplex, autoneg. > > * > > * @return > > + * On success: 0 > > * On failure: -1 > > - * Previous link state == linkdown: 0 > > - * Previous link state == linkup: 1 > > */ > > This was useful to detect the change in the status and log according. > With current change KNI example will keep logging the link state. > > Also in the KNI sample it periodically reads the link status from the > device and > applies it to the kni device, with current implementation it will keep > updating > the link status. Can we add a logic in the KNI sample so that update the > KNI > interface only if physical device link status changed? > We can still read from the carrier file and return the old state if it's really needed. I'll check the KNI sample code and try to find the best solution. > > > __rte_experimental > > int > > -rte_kni_update_link(struct rte_kni *kni, unsigned int linkup); > > +rte_kni_update_link(struct rte_kni *kni, struct rte_eth_link *link); > > > > /** > > * Close KNI device. > > diff --git a/lib/kni/rte_kni_common.h b/lib/kni/rte_kni_common.h > > index b547ea550171..35a1620e0bd2 100644 > > --- a/lib/kni/rte_kni_common.h > > +++ b/lib/kni/rte_kni_common.h > > @@ -130,10 +130,19 @@ struct rte_kni_device_info { > > uint8_t iova_mode; > > }; > > > > +struct rte_kni_link_info { > > + char name[RTE_KNI_NAMESIZE]; > > + unsigned int speed; > > + unsigned char duplex; > > + unsigned char autoneg; > > + unsigned char status; > > +}; > > Should we prefer fixed size storage types "uint32_t speed" or "unsigned > int", > did you select current ones explicitly? > Sure, I'll update. Not sure why I chose it initially, it was a long time ago :) > > > + > > #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_ */ > > > >