Re: [PATCH] net/pcap: reduce time for stopping device

[Ferruh Yigit <ferruh.yigit@xilinx.com>]

On 8/25/2022 12:17 PM, Zhou, YidingX wrote:
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@xilinx.com>
>> Sent: Thursday, August 25, 2022 6:09 PM
>> To: Zhou, YidingX <yidingx.zhou@intel.com>; dev@dpdk.org
>> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; stable@dpdk.org
>> Subject: Re: [PATCH] net/pcap: reduce time for stopping device
>>
>> On 8/25/2022 8:20 AM, Yiding Zhou wrote:
>>> The pcap file will be synchronized to the disk when stopping the device.
>>> It takes a long time if the file is large that would cause the 'detach
>>> sync request' timeout when the device is closed under multi-process
>>> scenario.
>>>
>>> This commit fixes the issue by performing synchronization in Tx path
>>>
>>> Fixes: 4c173302c307 ("pcap: add new driver")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Yiding Zhou <yidingx.zhou@intel.com>
>>> ---
>>>    drivers/net/pcap/pcap_ethdev.c | 18 ++++++++++++++++--
>>>    1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/pcap/pcap_ethdev.c
>>> b/drivers/net/pcap/pcap_ethdev.c index ec29fd6bc5..52eafa5674 100644
>>> --- a/drivers/net/pcap/pcap_ethdev.c
>>> +++ b/drivers/net/pcap/pcap_ethdev.c
>>> @@ -3,7 +3,7 @@
>>>     * Copyright(c) 2014 6WIND S.A.
>>>     * All rights reserved.
>>>     */
>>> -
>>> +#include <unistd.h>
>>>    #include <time.h>
>>>
>>>    #include <pcap.h>
>>> @@ -38,6 +38,8 @@
>>>
>>>    #define RTE_PMD_PCAP_MAX_QUEUES 16
>>>
>>> +#define ETH_PCAP_SYNC_THRESHOLD 0x20000000
>>> +

I guess this is 512MB, can you please comment this.
Is there any specific reason to select this value, or is it arbitrary?


>>>    static char errbuf[PCAP_ERRBUF_SIZE];
>>>    static struct timespec start_time;
>>>    static uint64_t start_cycles;
>>> @@ -47,6 +49,8 @@ static uint8_t iface_idx;
>>>    static uint64_t timestamp_rx_dynflag;
>>>    static int timestamp_dynfield_offset = -1;
>>>
>>> +RTE_DEFINE_PER_LCORE(uint64_t, _pcap_cached_bytes);
>>> +
>>>    struct queue_stat {
>>>    	volatile unsigned long pkts;
>>>    	volatile unsigned long bytes;
>>> @@ -144,6 +148,16 @@ static struct rte_eth_link pmd_link = {
>>>
>>>    RTE_LOG_REGISTER_DEFAULT(eth_pcap_logtype, NOTICE);
>>>
>>> +static inline void
>>> +pcap_dumper_data_sync(pcap_dumper_t *dumper, uint32_t bytes) {

'pcap_' is the namespace for the libpcap, can you select another prefix, 
like 'eth_pcap_' as many driver functions does.

>>> +	RTE_PER_LCORE(_pcap_cached_bytes) += bytes;
>>> +	if (unlikely(RTE_PER_LCORE(_pcap_cached_bytes) >
>> ETH_PCAP_SYNC_THRESHOLD)) {
>>> +		if (!fdatasync(fileno(pcap_dump_file(dumper))))
>>> +			RTE_PER_LCORE(_pcap_cached_bytes) = 0;
>>> +	}
>>> +}
>>> +

pcap supports multiple queue, and each queue creates a new pcap dumper 
and single core/thread can be used for this multiple dumpers. In that 
case I think above per lcore variable logic doesn't work.

And instead of having a global value, what do you think to add a 
variable to 'struct pcap_tx_queue' for this purpose?

>>>    static struct queue_missed_stat*
>>>    queue_missed_stat_update(struct rte_eth_dev *dev, unsigned int qid)
>>>    {
>>> @@ -421,7 +435,7 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf
>> **bufs, uint16_t nb_pkts)
>>>    	 * process stops and to make sure the pcap file is actually written,
>>>    	 * we flush the pcap dumper within each burst.
>>>    	 */
>>> -	pcap_dump_flush(dumper);
>>> +	pcap_dumper_data_sync(dumper, tx_bytes);
>>
>> 'pcap_dump_flush()' should be doing the same thing, to write buffer to file,
>> isn't it working?
>>
>> Can you check the return value of the 'pcap_dump_flush()' API, I wonder if it
>> keeps failing, for some reason?
>>
> 
> 'pcap_dump_flush()' returns 0 each time without error, it calls 'fflush()' to flush userspace buffers to kernel buffers, not disk. 'fdatasync()' to ensure data is written to disk.
> 

'pcap_dump_flush()' API documentation says "flushes the output buffer to 
the ``savefile,''", but as you said it uses 'fflush()' internally, so 
there is a chance that data is not written to the disk.

In this case, won't need to keep both, first flush and later 
fsync/fdatasync?

Do you observe any performance difference after change, since now 
writing to actual disk on datapath?

>>>    	dumper_q->tx_stat.pkts += num_tx;
>>>    	dumper_q->tx_stat.bytes += tx_bytes;
>>>    	dumper_q->tx_stat.err_pkts += nb_pkts - num_tx;
>