Re: [dpdk-dev] [PATCH v2 4/5] net/mlx5: add mlx5 APIs for single flow dump feature

[Haifei Luo <haifeil@nvidia.com>]

HI Slava,
     Yes for   "we define 36/72 bytes array?". 
     Correction for my last comment,   not byte , and it is  one "int" for port_id , 8 "int" for flowptr.
Sorry for the possible confusion.


-----Original Message-----
From: Haifei Luo 
Sent: Tuesday, April 13, 2021 9:29 AM
To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
Cc: Ori Kam <orika@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>; Xueming(Steven) Li <xuemingl@nvidia.com>; Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>
Subject: RE: [PATCH v2 4/5] net/mlx5: add mlx5 APIs for single flow dump feature

Hi Slava,
      For #1, The steering tool send messages to DPDK to request dump. Server/Client  use data structure "struct msghdr"
to communicate. It has " msg_iov " ," msg_iovlen" and etc. 
      In the tool side, Msg_iov is constructed as 1 byte for port_id,  8 bytes for flowptr. In DPDK, then we parse the message this way.  
      For #2,  I will move them to the beginning.

-----Original Message-----
From: Slava Ovsiienko <viacheslavo@nvidia.com>
Sent: Monday, April 12, 2021 3:38 PM
To: Haifei Luo <haifeil@nvidia.com>; dev@dpdk.org
Cc: Ori Kam <orika@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>; Xueming(Steven) Li <xuemingl@nvidia.com>; Haifei Luo <haifeil@nvidia.com>; Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>
Subject: RE: [PATCH v2 4/5] net/mlx5: add mlx5 APIs for single flow dump feature

> -----Original Message-----
> From: Haifei Luo <haifeil@nvidia.com>
> Sent: Wednesday, April 7, 2021 9:09
> To: dev@dpdk.org
> Cc: Ori Kam <orika@nvidia.com>; Slava Ovsiienko 
> <viacheslavo@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>;
> Xueming(Steven) Li <xuemingl@nvidia.com>; Haifei Luo 
> <haifeil@nvidia.com>; Matan Azrad <matan@nvidia.com>; Shahaf Shuler 
> <shahafs@nvidia.com>
> Subject: [PATCH v2 4/5] net/mlx5: add mlx5 APIs for single flow dump 
> feature
> 
> Modify API mlx5_flow_dev_dump to support the feature.
> Modify mlx5_socket since one extra arg flow_ptr is added.
> 
> Signed-off-by: Haifei Luo <haifeil@nvidia.com>

Sorry, this patch is errorneously acked instead of the
"common/mlx5: add mlx5 APIs for single flow dump feature"

I have comment for this one.

> +#ifndef _GNU_SOURCE
> +#define _GNU_SOURCE
> +#endif
> +
>  #include <sys/types.h>
>  #include <sys/socket.h>
>  #include <sys/un.h>
> @@ -29,11 +33,15 @@
>  {
>  	int conn_sock;
>  	int ret;
> +	int j;
>  	struct cmsghdr *cmsg = NULL;
> -	int data;
> +	#define LENGTH 9
> +	/* The first byte for port_id and the rest for flowptr. */
> +	int data[LENGTH];

So, we define 36/72 bytes array? And then use each int as byte to save flow_idx value?
I suppose the correct way would be to define the structure of message in stead of using ints array, something likle this:

struct mlx5_ipc_msg {
  int status;
  void* flow_idx;
}

> +	/* The first byte in data for port_id and the following 8 for flowptr */
> +	for (j = 1; j < LENGTH; j++)
> +		flow_ptr = (flow_ptr << 8) + data[j];
    If structure is define, there should be:
        flow_ptr =  msg->flow_idx

> +	if (flow_ptr == 0)
> +		ret = mlx5_flow_dev_dump(dev, NULL, file, NULL);
> +	else
> +		ret = mlx5_flow_dev_dump(dev,
> +			(struct rte_flow *)((uintptr_t)flow_ptr), file, &err);
> +

> +	/*dump one*/
> +	uint32_t handle_idx;
> +	int ret;
> +	struct mlx5_flow_handle *dh;
> +	struct rte_flow *flow = mlx5_ipool_get(priv->sh->ipool
> +		[MLX5_IPOOL_RTE_FLOW], (uintptr_t)(void *)flow_idx);
> +
Please, move variable declarations to the routine beginning, to others

With best regards, Slava