Re: [dpdk-dev] [PATCH v4 1/2] kni: rework rte_kni_update_link using ioctl

[Igor Ryzhov <iryzhov@nfware.com>]

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 <ferruh.yigit@intel.com> 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 <dg@adax.com>
> > Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
> > ---
> >  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 <rte_memory.h>
> >  #include <rte_mempool.h>
> >  #include <rte_ether.h>
> > +#include <rte_ethdev.h>
> >
> >  #include <rte_kni_common.h>
> >
> > @@ -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_ */
> >
>
>