On 6/27/2018 1:04 PM, ido goshen wrote:
> From: ido g <i...@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 <i...@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>");
>
>
