On 6/5/2019 1:46 PM, Ferriter, Cian wrote:
> Adding in my changelog at the top of this, since I forgot to add it in the
> original mail:
>
> v2:
> * Rework the method of filling the ring to infinitely rx from
> * Avoids potential huge allocation of mbufs
> * Removes double allocation of mbufs used during queue setup
> * rename count_packets_in_pcaps to count_packets_in_pcap
> * initialize pcap_pkt_count in count_packets_in_pcap
> * use RTE_PMD_REGISTER_PARAM_STRING <0|1> rather than <int>
> * replace calls to rte_panic with proper error returning
> * count rx and tx stat bytes in pcap_rx_infinite and tx_drop
> * make internals->infinite_rx = infinite_rx assignment unconditional
> * add cleanup for infinite_rx in eth_dev_close and pmd_pcap_remove
> * add cleanup when multi seg mbufs are found
> * add some clarifications to the documentation update
>
>> -----Original Message-----
>> From: Ferriter, Cian
>> Sent: 05 June 2019 12:56
>> To: Richardson, Bruce <bruce.richard...@intel.com>; Yigit, Ferruh
>> <ferruh.yi...@intel.com>; Mcnamara, John <john.mcnam...@intel.com>;
>> Kovacevic, Marko <marko.kovace...@intel.com>
>> Cc: dev@dpdk.org; Ferriter, Cian <cian.ferri...@intel.com>
>> Subject: [PATCH 19.08 v2] net/pcap: enable infinitely rxing a pcap file
>>
>> It can be useful to use pcap files for some rudimental performance testing.
>> This patch enables this functionality in the pcap driver.
>>
>> At a high level, this works by creaing a ring of sufficient size to store the
>> packets in the pcap file passed to the application. When the rx function for
>> this mode is called, packets are dequeued from the ring for use by the
>> application and also enqueued back on to the ring to be "received" again.
>>
>> A tx_drop mode is also added since transmitting to a tx_pcap file isn't
>> desirable at a high traffic rate.
>>
>> Jumbo frames are not supported in this mode. When filling the ring at rx
>> queue setup time, the presence of multi segment mbufs is checked for.
>> The PMD will exit on detection of these multi segment mbufs.
>>
>> Signed-off-by: Cian Ferriter <cian.ferri...@intel.com>
>> ---
>> doc/guides/nics/pcap_ring.rst | 19 +++
>> drivers/net/pcap/rte_eth_pcap.c | 268
>> ++++++++++++++++++++++++++++++--
>> 2 files changed, 277 insertions(+), 10 deletions(-)
>>
>> diff --git a/doc/guides/nics/pcap_ring.rst b/doc/guides/nics/pcap_ring.rst
>> index c1ef9196b..b272e6fe3 100644
>> --- a/doc/guides/nics/pcap_ring.rst
>> +++ b/doc/guides/nics/pcap_ring.rst
>> @@ -106,6 +106,25 @@ Runtime Config Options
>>
>> --vdev 'net_pcap0,iface=eth0,phy_mac=1'
>>
>> +- Use the RX PCAP file to infinitely receive packets
>> +
>> + In case ``rx_pcap=`` configuration is set, user may want to use the
>> + selected PCAP file for rudimental performance testing. This can be done
>> with a ``devarg`` ``infinite_rx``, for example::
>> +
>> + --vdev 'net_pcap0,rx_pcap=file_rx.pcap,infinite_rx=1,tx_drop=1'
Can be good to highlight that this flag is not per queue, but should be provided
once (explictly once since code checks it) per Rx.
>> +
>> + When this mode is used, it is recommended to use the ``tx_drop``
>> ``devarg``.
>> +
>> + This option is device wide, so all queues on a device will either have this
>> enabled or disabled.
>> +
>> +- Drop all packets on transmit
>> +
>> + The user may want to drop all packets on tx for a device. This can be done
>> with the ``tx_drop`` ``devarg``, for example::
>> +
>> + --vdev 'net_pcap0,rx_pcap=file_rx.pcap,tx_drop=1'
>> +
>> + One tx drop queue is created for each rxq on that device.
Can we drop the ``tx_drop`` completely?
What happens when no 'tx_pcap' or 'tx_iface' provided at all, to imply the
tx_drop?
<...>
>> @@ -1105,7 +1290,8 @@ static int
>> eth_from_pcaps(struct rte_vdev_device *vdev,
>> struct pmd_devargs *rx_queues, const unsigned int
>> nb_rx_queues,
>> struct pmd_devargs *tx_queues, const unsigned int
>> nb_tx_queues,
>> - int single_iface, unsigned int using_dumpers)
>> + int single_iface, unsigned int using_dumpers,
>> + unsigned int infinite_rx, unsigned int tx_drop)
The argument list is keep increasing. What happens is 'pmd_pcap_probe()'
processes the user input (devargs) and passes the processed output to this
function to create ethdev.
What do you think gathering all processed output to a struct and pass it the
this function, in a patch before this patch? Like:
struct pmd_devargs_all {
struct pmd_devargs pcaps;
struct pmd_devargs dumpers;
int single_iface;
unsigned intis_tx_pcap;
};
And add 'unsigned int infinite_rx;' into that struct in this patch.
<...>
>> @@ -1148,6 +1342,7 @@ pmd_pcap_probe(struct rte_vdev_device *dev) {
>> const char *name;
>> unsigned int is_rx_pcap = 0, is_tx_pcap = 0;
>> + unsigned int infinite_rx = 0, infinite_rx_arg_cnt = 0, tx_drop = 0;
Is initial value required for 'infinite_rx_arg_cnt '?
>> struct rte_kvargs *kvlist;
>> struct pmd_devargs pcaps = {0};
>> struct pmd_devargs dumpers = {0};
>> @@ -1216,7 +1411,25 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
>> is_rx_pcap = rte_kvargs_count(kvlist, ETH_PCAP_RX_PCAP_ARG) ? 1
>> : 0;
>> pcaps.num_of_queue = 0;
>>
>> + infinite_rx_arg_cnt = rte_kvargs_count(kvlist,
>> + ETH_PCAP_INFINITE_RX_ARG);
Can move this under 'is_rx_pcap', since this value only make sense when Rx is
pcap.
>> +
>> if (is_rx_pcap) {
>> + /*
>> + * We check whether we want to infinitely rx the pcap file.
>> + */
>> + if (infinite_rx_arg_cnt == 1) {
>> + ret = rte_kvargs_process(kvlist,
>> + ETH_PCAP_INFINITE_RX_ARG,
>> + &get_infinite_rx_arg, &infinite_rx);
>> + if (ret < 0)
>> + goto free_kvlist;
>> + } else if (infinite_rx_arg_cnt >= 1) {
I guess it should be ">" instead of ">="
<...>
>> @@ -1285,8 +1512,12 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
>> goto free_kvlist;
>> }
>>
>> + PMD_LOG(INFO, "Configure pmd_pcap: infinite_rx is %s",
>> + infinite_rx ? "enabled" : "disabled");
What do you think printing the message when feature is requested? Instead of
printing each time that it is disabled, which is default behaviour.
<...>
>> @@ -1318,6 +1550,20 @@ pmd_pcap_remove(struct rte_vdev_device *dev)
>> eth_dev->data->mac_addrs = NULL;
>> }
>>
>> + /* Device wide flag, but cleanup must be performed per queue. */
>> + if (internals->infinite_rx) {
>> + for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>> + struct pcap_rx_queue *pcap_q = &internals-
>>> rx_queue[i];
>> + struct rte_mbuf *pcap_buf;
>> +
>> + while (!rte_ring_dequeue(pcap_q->pkts,
>> + (void **)&pcap_buf))
>> + rte_pktmbuf_free(pcap_buf);
>> +
>> + rte_ring_free(pcap_q->pkts);
>> + }
>> + }
Can it possible to call 'eth_dev_close()' which seems dublicating the above
code?
