Hi Ferruh,
Thanks for the review.
I've send a v3 and responded to your comments below.
Thanks,
Cian
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: 12 June 2019 15:10
> To: Ferriter, Cian <cian.ferri...@intel.com>; Richardson, Bruce
> <bruce.richard...@intel.com>; Mcnamara, John
> <john.mcnam...@intel.com>; Kovacevic, Marko
> <marko.kovace...@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 19.08 v2] net/pcap: enable infinitely rxing a pcap file
>
> 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.
>
Added to the docs in the next version.
> >> +
> >> + 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?
>
This sound like nice default behavior to have. I've updated the latest version
to implement this and I've removed the tx_drop args parsing and related doc
section.
> <...>
>
> >> @@ -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.
>
Good idea, I've implemented these changes in the next version.
> <...>
>
> >> @@ -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 '?
>
I don't think the initial value is required. I was just following the
convention used with the 'is_rx_pcap' and 'is_tx_pcap' variables.
I think that the below comment takes care of this, since we can move
'infinite_rx_arg_cnt' under 'is_rx_pcap' and assign directly to the output of
the 'rte_kvargs_count'.
> >> 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.
>
True, that makes sense. Fixed in the next version.
> >> +
> >> 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 ">="
>
Ooops, you are correct. The '=' part is pointless here. Fixed in the next
version.
> <...>
>
> >> @@ -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.
>
I agree, there's no point in mentioning that infinite_rx has been disabled if
the user hasn't requested it in the args.
Fixed in the new version.
> <...>
>
> >> @@ -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?
This makes sense. Added this in the next version.
