Re: [dpdk-dev] [PATCH v2 3/3] examples/ethtool: add control interface support to the application

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Ferruh,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Friday, February 12, 2016 1:46 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 3/3] examples/ethtool: add control interface support to the application
> 
> Control interface APIs added into the sample application.
> 
> To have the support corresponding kernel module (KCP) needs to be inserted.
> If kernel module is not there, application will run as it is without
> kernel control path support.
> 
> When KCP module inserted, running application creates a virtual Linux
> network interface (dpdk$) per DPDK port. This interface can be used by
> traditional Linux tools.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  doc/guides/sample_app_ug/ethtool.rst | 41 ++++++++++++++++++++++++++++++++++++
>  examples/ethtool/ethtool-app/main.c  | 10 +++++++--
>  2 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guides/sample_app_ug/ethtool.rst b/doc/guides/sample_app_ug/ethtool.rst
> index 4d1697e..2174288 100644
> --- a/doc/guides/sample_app_ug/ethtool.rst
> +++ b/doc/guides/sample_app_ug/ethtool.rst
> @@ -131,6 +131,47 @@ application`_. Individual call-back functions handle the detail
>  associated with each command, which make use of the functions
>  defined in the `Ethtool interface`_ to the DPDK functions.

There is ~100% code duplication between
lib/librte_ctrl_if/rte_ethtool.c and examples/ethtool/lib/rte_ethtool.c
That need to be addressed somehow.

> 
> +Control Interface
> +~~~~~~~~~~~~~~~~~
> +
> +If Kernel Control Path (KCP) kernel module (rte_kcp.ko) inserted,
> +virtual interfaces created for each DPDK port for control purposes.
> +
> +Created interfaces are named as dpdk#, like:
> +
> +.. code-block:: console
> +
> +        # ifconfig dpdk0; ifconfig dpdk1
> +        dpdk0: flags=4099<UP,BROADCAST,MULTICAST>  mtu 1500
> +                ether 90:e2:ba:0e:49:b9  txqueuelen 1000  (Ethernet)
> +                RX packets 0  bytes 0 (0.0 B)
> +                RX errors 0  dropped 0  overruns 0  frame 0
> +                TX packets 0  bytes 0 (0.0 B)
> +                TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
> +
> +        dpdk1: flags=4099<UP,BROADCAST,MULTICAST>  mtu 1500
> +                ether 00:1b:21:76:fa:21  txqueuelen 1000  (Ethernet)
> +                RX packets 0  bytes 0 (0.0 B)
> +                RX errors 0  dropped 0  overruns 0  frame 0
> +                TX packets 0  bytes 0 (0.0 B)
> +                TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
> +
> +Regular Linux commands can be issued on interfaces:
> +
> +.. code-block:: console
> +
> +        # ethtool -i dpdk0
> +        driver: rte_ixgbe_pmd
> +        version: RTE 2.3.0-rc0
> +        firmware-version:
> +        expansion-rom-version:
> +        bus-info: 0000:08:00.1
> +        supports-statistics: yes
> +        supports-test: no
> +        supports-eeprom-access: yes
> +        supports-register-dump: yes
> +        supports-priv-flags: no
> +
>  Ethtool interface
>  -----------------
> 
> diff --git a/examples/ethtool/ethtool-app/main.c b/examples/ethtool/ethtool-app/main.c
> index e21abcd..68b13ad 100644
> --- a/examples/ethtool/ethtool-app/main.c
> +++ b/examples/ethtool/ethtool-app/main.c
> @@ -1,7 +1,7 @@
>  /*-
>   *   BSD LICENSE
>   *
> - *   Copyright(c) 2015 Intel Corporation. All rights reserved.
> + *   Copyright(c) 2016 Intel Corporation. All rights reserved.
>   *   All rights reserved.
>   *
>   *   Redistribution and use in source and binary forms, with or without
> @@ -44,6 +44,7 @@
>  #include <rte_memory.h>
>  #include <rte_mempool.h>
>  #include <rte_mbuf.h>
> +#include <rte_ctrl_if.h>
> 
>  #include "ethapp.h"
> 
> @@ -54,7 +55,6 @@
>  #define PKTPOOL_EXTRA_SIZE 512
>  #define PKTPOOL_CACHE 32
> 
> -
>  struct txq_port {
>  	uint16_t cnt_unsent;
>  	struct rte_mbuf *buf_frames[MAX_BURST_LENGTH];
> @@ -254,6 +254,8 @@ static int slave_main(__attribute__((unused)) void *ptr_data)
>  			}
>  			rte_spinlock_unlock(&ptr_port->lock);
>  		} /* end for( idx_port ) */
> +		rte_eth_control_interface_process_msg(
> +				RTE_ETHTOOL_CTRL_IF_PROCESS_MSG, 0);


As I can see, few problems here:
1. Race condition was introduced between slave_main() and ethapp_main() -
both can try to do dev_start()/dev_stop() or other intrusive things over the same port  
simultaneously.
2. Better to avoid calling rte_eth_control_interface_process_msg() from RT code path
     that doing RX/TX packets - it is too slow for that.
3. Right now - if you'll have to postpone any RX/TX on any ports when calling  rte_eth_control_interface_process_msg().
     As it can't distinguish message for what particular port it is going to process.
    Need to address it somehow - either add function that would return current message port_id,
   or introduce a sync callback function and add is a parameter for  rte_eth_control_interface_process_msg() ,
  or probably something else.

Konstantin