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

[Ido Goshen <Ido@cgstowernetworks.com>]

The problem is if a dpdk app uses the same iface(s) both as rx_iface and tx_iface then it will receive back the packets it sends.
If my app sends a packet to portid=X with rte_eth_tx_burst() then I wouldn't expect to receive it back by rte_eth_rx_burst() for that same portid=X  (assuming of course there's no external loopback)
This is coming from the default nature of pcap that like a sniffer captures both incoming and outgoing direction.
The patch provides an option to limit pcap rx_iface to get only incoming traffic which is more like a real (non-pcap) dpdk device.

for example:
when using existing *rx_iface*
	l2fwd -c 3 -n1 --no-huge --vdev=eth_pcap0,rx_iface=eth1,tx_iface=eth1 --vdev=eth_pcap1,rx_iface=dummy0,tx_iface=dummy0  -- -p 3 -T 1 
sending only 1 single packet into eth1 will end in an infinite loop - 
the packet that is being transmitted by l2fwd to tx_iface=dummy0 is being recaptured again by the rx_iface=dummy0 (as it captures outgoing packets too) 
then l2fwd it to tx_iface=eth0 and it is received back on it by the rx_iface=eth0 and so on...
	Port statistics ====================================
	Statistics for port 0 ------------------------------
	Packets sent:                     4658
	Packets received:                 4658
	Packets dropped:                     0
	Statistics for port 1 ------------------------------
	Packets sent:                     4658
	Packets received:                 4658
	Packets dropped:                     0
	Aggregate statistics ===============================
	Total packets sent:               9316
	Total packets received:           9316
	Total packets dropped:               0
	====================================================

while if using the suggested *rx_iface_in*
	l2fwd -c 3 -n1 --no-huge --vdev=eth_pcap0,rx_iface_in=eth1,tx_iface=eth1 --vdev=eth_pcap1,rx_iface_in=dummy0,tx_iface=dummy0  -- -p 3 -T 1
The packet will be transmitted once and not received again (as I'd expect)
	Port statistics ====================================
	Statistics for port 0 ------------------------------
	Packets sent:                        0
	Packets received:                    1
	Packets dropped:                     0
	Statistics for port 1 ------------------------------
	Packets sent:                        1
	Packets received:                    0
	Packets dropped:                     0
	Aggregate statistics ===============================
	Total packets sent:                  1
	Total packets received:              1
	Total packets dropped:               0
	====================================================

-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com> 
Sent: Tuesday, June 5, 2018 4:27 PM
To: Ido Goshen <Ido@cgstowernetworks.com>
Cc: dev@dpdk.org
Subject: Re: [PATCH v2] net/pcap: rx_iface_in stream type support

On 6/5/2018 10:39 AM, ido goshen wrote:
> Support rx of in direction packets only Useful for apps that also tx 
> to eth_pcap ports in order not to see them echoed back in as rx when 
> out direction is also captured

Can you please explain the problem a little more?
If you are using rx_iface and tx_iface arguments you should have different pcap handlers for Rx and Tx and you shouldn't be getting Tx packets in Rx handler.

> 
> Signed-off-by: ido goshen <ido@cgstowernetworks.com>
> ---
> v2: clean checkpatch warning
> 
>  drivers/net/pcap/rte_eth_pcap.c | 36 
> ++++++++++++++++++++++++++++++++----

doc/guides/nics/pcap_ring.rst also needs to be updated with this new arg, but please wait to clarify the above comment, to figure out if this patch really necessary, before updating that document.

>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/pcap/rte_eth_pcap.c 
> b/drivers/net/pcap/rte_eth_pcap.c index 6bd4a7d..132f469 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
> @@ -726,6 +728,22 @@ struct pmd_devargs {
>  	return 0;
>  }
>  
> +static inline int
> +set_iface_direction(const char *iface, const char *key, pcap_t *pcap) 
> +{
> +	if (strcmp(key, ETH_PCAP_RX_IFACE_IN_ARG) == 0) {

set_iface_direction() can be more generic function, instead of checking the key here, it can be done the functions calls this.

Also can be possible to make even more generic, this function only sets direction as PCAP_D_IN, you can get in/out as input to function.

> +		if (pcap_setdirection(pcap, PCAP_D_IN) < 0) {
> +			PMD_LOG(ERR,
> +				"Setting %s pcap direction IN failed - %s\n",
> +				 iface,
> +				 pcap_geterr(pcap));
> +			return -1;
> +		}
> +		PMD_LOG(INFO, "Setting %s pcap direction IN\n", iface);
> +	}
> +	return 0;
> +}
> +
>  /*
>   * Opens a NIC for reading packets from it
>   */
> @@ -740,11 +758,12 @@ struct pmd_devargs {
>  	for (i = 0; i < rx->num_of_queue; i++) {
>  		if (open_single_iface(iface, &pcap) < 0)
>  			return -1;
> +		if (set_iface_direction(iface, key, pcap) < 0)
> +			return -1;
>  		rx->queue[i].pcap = pcap;
>  		rx->queue[i].name = iface;
>  		rx->queue[i].type = key;
>  	}
> -
>  	return 0;
>  }
>  
> @@ -963,17 +982,25 @@ struct pmd_devargs {
>  		is_rx_pcap = 1;
>  	else
>  		pcaps.num_of_queue = rte_kvargs_count(kvlist,
> -				ETH_PCAP_RX_IFACE_ARG);
> +				ETH_PCAP_RX_IFACE_ARG) +
> +				rte_kvargs_count(kvlist,
> +						ETH_PCAP_RX_IFACE_IN_ARG);
>  
>  	if (pcaps.num_of_queue > RTE_PMD_PCAP_MAX_QUEUES)
>  		pcaps.num_of_queue = RTE_PMD_PCAP_MAX_QUEUES;
>  
> -	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);

I guess there is an assumption here ETH_PCAP_RX_IFACE_ARG and ETH_PCAP_RX_IFACE_IN_ARG are mutually exclusive. What if user uses both?
Anything to protect against this, or any information to prevent this usage?

> +	}
>  
>  	if (ret < 0)
>  		goto free_kvlist;
> @@ -1046,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>");
>  
>