From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by inbox.dpdk.org (Postfix) with ESMTP id ECF3AA0613
	for <public@inbox.dpdk.org>; Wed, 25 Sep 2019 11:00:54 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id 3860C2BB5;
	Wed, 25 Sep 2019 11:00:53 +0200 (CEST)
Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com
 [209.85.128.65]) by dpdk.org (Postfix) with ESMTP id C4C432BAB
 for <dev@dpdk.org>; Wed, 25 Sep 2019 11:00:51 +0200 (CEST)
Received: by mail-wm1-f65.google.com with SMTP id b24so3706439wmj.5
 for <dev@dpdk.org>; Wed, 25 Sep 2019 02:00:51 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=nfware-com.20150623.gappssmtp.com; s=20150623;
 h=mime-version:references:in-reply-to:from:date:message-id:subject:to
 :cc; bh=VKcXHyqdeL4t+qTRhCAc0JO9Wa0QoP+iJ1iqDqWxmUA=;
 b=BcRUkj4XgAuOh8W4BEAGKUikpDimw09OyFZpJJE3avrgwp1jLWTbEptwhCMhc83Zd3
 OoeXk0NxZA0G0qr6iaD37HDzVLKm2e2CZQm/fmCqXnl8VCGbB4/dJbGChTBqdvYlDh3W
 hz9J4SLLhOBczxIC07za5QU24JUEv9YW9je4EFMchLtD5eSBqxTuPjwLwNF/Pts7Ub1t
 K1UJ4ILBN0Ui5ZU2y+t+H4knX8+dW7auxAujnHXKzjfOrMrO4vtEwpW3OYcmdXO+IiiZ
 p24zncXrS+OJYoxqo91HA+a0ILj8pbAFAbWDvsnvXCErhG4zuBRfp+clO++c3CGcnT2K
 20yQ==
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=VKcXHyqdeL4t+qTRhCAc0JO9Wa0QoP+iJ1iqDqWxmUA=;
 b=kIqihAVEeIK11ahwPSNaw2S7VvvnUv56UiJI5gJe/vg3n3FR0Oj/CAyQLuHhr8zxGh
 v72Qk4prJNLp5p1CObvdYMt5uCtELcGGD5o3rxfwP0lAe5qJ0d/zdYPqOwIoxwNBYwAr
 XmarKcipKPyHcvvFR2uS3dseCrhh/l23jsS/tU+eqIJ2eHZOWzGSmYEFV0Vs9ffIV5nn
 UIaABKEd+RjHFVQFv19rGZW6Cwee2Km3fyyb2gXqIuTYY2FDLv2Pew0O+xAt5RRkqSbj
 uCUjH4SAzNcyAXI2OBRWKtgIuePq8ERsHVptOaiSDF6189Cw1Oi6RfQM1IBf3CtTg4jh
 PRmw==
X-Gm-Message-State: APjAAAVV+ic6+pWa3K4d3PqvcOBZjEBMVOINIlH0OIB789mwu/l0J+8c
 XPccOu0Pq8RIVslr0qfrIpwuedNkkOPZLwRyMZDZR88BaD7zrw==
X-Google-Smtp-Source: APXvYqyLxvkKoCucwzt1X+ohqdmoIVcF2pvS/Kh0ggijoBoyREtMIZMG/U2+gnLZHQqOs4jw+cmlm2Lfmn8CTktAF0M=
X-Received: by 2002:a7b:c4c7:: with SMTP id g7mr6202768wmk.11.1569402051062;
 Wed, 25 Sep 2019 02:00:51 -0700 (PDT)
MIME-Version: 1.0
References: <20190924192548.17291-1-iryzhov@nfware.com>
 <20190924193312.17381-1-iryzhov@nfware.com>
 <f7tr245juqn.fsf@dhcp-25.97.bos.redhat.com>
In-Reply-To: <f7tr245juqn.fsf@dhcp-25.97.bos.redhat.com>
From: Igor Ryzhov <iryzhov@nfware.com>
Date: Wed, 25 Sep 2019 12:00:39 +0300
Message-ID: <CAF+s_FzV7+zcZdqvoqQ=nC7_y+sfwkuFbGEZTY2YvnQGe_7wEQ@mail.gmail.com>
To: Aaron Conole <aconole@redhat.com>
Cc: dev@dpdk.org
Content-Type: text/plain; charset="UTF-8"
X-Content-Filtered-By: Mailman/MimeDel 2.1.15
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Sure, my bad, sorry. Will send v3.

On Tue, Sep 24, 2019 at 11:37 PM Aaron Conole <aconole@redhat.com> wrote:

> 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
>