[v3] net/pcap: rx_iface_in stream type support

  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

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(-)
  

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>");
>  
>
  

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, June 27, 2018 4:59 PM
> To: Ido Goshen <Ido@cgstowernetworks.com>; Bruce Richardson
> <bruce.richardson@intel.com>; John McNamara
> <john.mcnamara@intel.com>; Marko Kovacevic
> <marko.kovacevic@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v3] net/pcap: rx_iface_in stream type support
> 
> 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.

[idog] I think one can just use the new doc example below (the one w/o the _in option) but I can add it in the commit log too...
> 
> 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):
> 

[idog] No, This is true indifferent to what the other params are.
i.e. 
In case iface_rx is given the dpdk app will see not only packets coming into that iface (e.g. echo request) but also what linux apps are sending out of that iface (e.g. echo reply)
In case iface_rx_in is given it will see only incoming traffic (only the echo requests)
Giving tx_iface with the same iface just exposes that behavior and makes it worst cause it will also capture back what the dpdk app is sending to that iface and not only what Linux sends.
Therefore I think the documentation is correct.

> "
> 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.

[idog] Would this make it clearer?
"Read only incoming packets from a network interface and write them back to that network interface:"

> 
> > +
> > +.. 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)
[idog] will fix this as well as all the indentations and white spaces comments below

> 
> > +{
> > +	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?
[idog] Good catch, they need to be parsed together. I'll fix it.

> 
> > +	}
> >
> >  	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>");
> >
> >