Re: [dpdk-dev] [PATCH v3] net/pcap: rx_iface_in stream type support

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 6/27/2018 1:04 PM, ido goshen wrote:
> From: ido g <ido@cgstowernetworks.com>
> 
> Support rx of in direction packets only
> Useful for apps that also tx to eth_pcap ports in order to not see them
> echoed back in as rx when out direction is also captured

Can you please add your command, which was in previous mails, on how to
re-produce the issue of capturing transferred packets in Rx path; for future.

And overall looks good, there are a few syntax comments below.

> 
> Signed-off-by: ido g <ido@cgstowernetworks.com>
> ---
> v3:
> * merge to updated dpdk-next-net code
> * pcap_ring doc update
> 
> v2:
> * clean checkpatch warning
> 
>  doc/guides/nics/pcap_ring.rst   | 25 ++++++++++++++++++++++-
>  drivers/net/pcap/rte_eth_pcap.c | 45 ++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/guides/nics/pcap_ring.rst b/doc/guides/nics/pcap_ring.rst
> index 7fd063c..6282be6 100644
> --- a/doc/guides/nics/pcap_ring.rst
> +++ b/doc/guides/nics/pcap_ring.rst
> @@ -71,11 +71,19 @@ The different stream types are:
>          tx_pcap=/path/to/file.pcap
>  
>  *   rx_iface: Defines a reception stream based on a network interface name.
> -    The driver reads packets coming from the given interface using the Linux kernel driver for that interface.
> +    The driver reads packets from the given interface using the Linux kernel driver for that interface.
> +    The driver captures both the incoming and outgoing packets on that interface.

This is only true if tx_iface parameter given for that interface, right? I can
be good to clarify to not confuse people. I am for keeping first sentences, and
add a note about this special case, something like (feel free to update):

"
The driver reads packets coming from the given interface using the Linux kernel
driver for that interface.
When tx_iface argument given for same interface, Tx packets also captured.
"

>      The value is an interface name.
>  
>          rx_iface=eth0
>  
> +*   rx_iface_in: Defines a reception stream based on a network interface name.
> +    The driver reads packets from the given interface using the Linux kernel driver for that interface.
> +    The driver captures only the incoming packets on that interface.

Again I am for keeping "... reads packets coming from the given interface ..."
and clarify the difference in next sentences specific to tx_iface usage.

> +    The value is an interface name.
> +
> +        rx_iface_in=eth0
> +
>  *   tx_iface: Defines a transmission stream based on a network interface name.
>      The driver sends packets to the given interface using the Linux kernel driver for that interface.
>      The value is an interface name.
> @@ -122,6 +130,21 @@ Forward packets through two network interfaces:
>      $RTE_TARGET/app/testpmd -l 0-3 -n 4 \
>          --vdev 'net_pcap0,iface=eth0' --vdev='net_pcap1;iface=eth1'
>  
> +Enable 2 tx queues on a network interface:> +
> +.. code-block:: console
> +
> +    $RTE_TARGET/app/testpmd -l 0-3 -n 4 \
> +        --vdev 'net_pcap0,rx_iface=eth1,tx_iface=eth1,tx_iface=eth1' \
> +        -- --txq 2
> +
> +Read only incoming packets from a network interface:

This title is confusing, the sample is not for "read only incoming packets" it
Tx also J. I understand what you mean, but I believe it would be better to
clarify this.

> +
> +.. code-block:: console
> +
> +    $RTE_TARGET/app/testpmd -l 0-3 -n 4 \
> +        --vdev 'net_pcap0,rx_iface_in=eth1,tx_iface=eth1'
> +
>  Using libpcap-based PMD with the testpmd Application
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index b21930b..9c31cef 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -26,6 +26,7 @@
>  #define ETH_PCAP_RX_PCAP_ARG  "rx_pcap"
>  #define ETH_PCAP_TX_PCAP_ARG  "tx_pcap"
>  #define ETH_PCAP_RX_IFACE_ARG "rx_iface"
> +#define ETH_PCAP_RX_IFACE_IN_ARG "rx_iface_in"
>  #define ETH_PCAP_TX_IFACE_ARG "tx_iface"
>  #define ETH_PCAP_IFACE_ARG    "iface"
>  
> @@ -83,6 +84,7 @@ struct pmd_devargs {
>  	ETH_PCAP_RX_PCAP_ARG,
>  	ETH_PCAP_TX_PCAP_ARG,
>  	ETH_PCAP_RX_IFACE_ARG,
> +	ETH_PCAP_RX_IFACE_IN_ARG,
>  	ETH_PCAP_TX_IFACE_ARG,
>  	ETH_PCAP_IFACE_ARG,
>  	NULL
> @@ -739,6 +741,26 @@ struct pmd_devargs {
>  }
>  
>  static inline int
> +set_iface_direction(const char *iface,
> +		pcap_t *pcap,
> +		pcap_direction_t direction)

Can you please follow same syntax in the code, like:
set_iface_direction(const char *iface, pcap_t *pcap,
	pcap_direction_t direction)

> +{
> +	const char *direction_str = (direction == PCAP_D_IN) ? "IN" : "OUT";
> +	if (pcap_setdirection(pcap, direction) < 0) {
> +		PMD_LOG(ERR,
> +				"Setting %s pcap direction %s failed - %s\n",
> +				iface,
> +				direction_str,
> +				pcap_geterr(pcap));

Can you please fix the indentations above, lines can be joined:

PMD_LOG(ERR, "Setting %s pcap direction %s failed - %s\n",
	iface, direction_str, pcap_geterr(pcap));

> +		return -1;
> +	}
> +	PMD_LOG(INFO, "Setting %s pcap direction %s\n",
> +			iface,
> +			direction_str);

Same here, one tab is enough for next line and can join lines

> +	return 0;
> +}
> +
> +static inline int
>  open_iface(const char *key, const char *value, void *extra_args)
>  {
>  	const char *iface = value;
> @@ -761,7 +783,17 @@ struct pmd_devargs {
>  static inline int
>  open_rx_iface(const char *key, const char *value, void *extra_args)
>  {
> -	return open_iface(key, value, extra_args);
> +	int ret = open_iface(key, value, extra_args);
> +	if (ret < 0)	
> +		return ret;
> +	if (strcmp(key, ETH_PCAP_RX_IFACE_IN_ARG) == 0) {
> +		struct pmd_devargs *pmd = extra_args;
> +		unsigned int qid = pmd->num_of_queue - 1;
> +		set_iface_direction(pmd->queue[qid].name,
> +				pmd->queue[qid].pcap,
> +				PCAP_D_IN);
> +	}

Can you please put an empty line after variables and before return.

> +	return 0;
>  }
>  
>  /*
> @@ -964,12 +996,18 @@ struct pmd_devargs {
>  	is_rx_pcap = rte_kvargs_count(kvlist, ETH_PCAP_RX_PCAP_ARG) ? 1 : 0;
>  	pcaps.num_of_queue = 0;
>  
> -	if (is_rx_pcap)
> +	if (is_rx_pcap) {
>  		ret = rte_kvargs_process(kvlist, ETH_PCAP_RX_PCAP_ARG,
>  				&open_rx_pcap, &pcaps);
> -	else
> +	} else {
>  		ret = rte_kvargs_process(kvlist, ETH_PCAP_RX_IFACE_ARG,
>  				&open_rx_iface, &pcaps);
> +		if (ret == 0)
> +			ret = rte_kvargs_process(kvlist,
> +					ETH_PCAP_RX_IFACE_IN_ARG,
> +					&open_rx_iface,
> +					&pcaps);

Here if RX_IFACE_ARG and RX_IFACE_IN_ARG used mixed (nothing seems prevents
this), the queue order can be not same as argument order, do you think is this a
problem?

> +	}
>  
>  	if (ret < 0)
>  		goto free_kvlist;
> @@ -1035,6 +1073,7 @@ struct pmd_devargs {
>  	ETH_PCAP_RX_PCAP_ARG "=<string> "
>  	ETH_PCAP_TX_PCAP_ARG "=<string> "
>  	ETH_PCAP_RX_IFACE_ARG "=<ifc> "
> +	ETH_PCAP_RX_IFACE_IN_ARG "=<ifc> "
>  	ETH_PCAP_TX_IFACE_ARG "=<ifc> "
>  	ETH_PCAP_IFACE_ARG "=<ifc>");
>  
>