[dpdk-dev] [PATCH 0/3] Multiseg fixes for pcap pmd

[dpdk-dev] [PATCH 0/3] Multiseg fixes for pcap pmd

[David Marchand]

Here are some fixes caught while looking at oerrors statistics for this
driver.
The second patch can be seen as a revert or a followup of [1].

1: https://git.dpdk.org/dpdk/commit/?id=49a0a2ffd5db

-- 
David Marchand

David Marchand (3):
  net/pcap: fix Rx with small buffers
  net/pcap: fix transmit return count in error conditions
  net/pcap: fix concurrent multiseg packet transmits

 drivers/net/pcap/rte_eth_pcap.c | 103 +++++++++++++++-------------------------
 1 file changed, 37 insertions(+), 66 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH 1/3] net/pcap: fix Rx with small buffers

[David Marchand]

If the pkt pool contains only buffers smaller than the default headroom,
then the driver will compute an invalid buffer size (negative value cast
to an uint16_t).
Rely on the mbuf api to check how much space is available in the mbuf.

Fixes: 6eb0ae218a98 ("pcap: fix mbuf allocation")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 322c18f..470867d 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -242,7 +242,6 @@ struct pmd_devargs_all {
 	struct rte_mbuf *mbuf;
 	struct pcap_rx_queue *pcap_q = queue;
 	uint16_t num_rx = 0;
-	uint16_t buf_size;
 	uint32_t rx_bytes = 0;
 	pcap_t *pcap;
 
@@ -265,11 +264,7 @@ struct pmd_devargs_all {
 		if (unlikely(mbuf == NULL))
 			break;
 
-		/* Now get the space available for data in the mbuf */
-		buf_size = rte_pktmbuf_data_room_size(pcap_q->mb_pool) -
-				RTE_PKTMBUF_HEADROOM;
-
-		if (header.caplen <= buf_size) {
+		if (header.caplen <= rte_pktmbuf_tailroom(mbuf)) {
 			/* pcap packet will fit in the mbuf, can copy it */
 			rte_memcpy(rte_pktmbuf_mtod(mbuf, void *), packet,
 					header.caplen);
-- 
1.8.3.1

[dpdk-dev] [PATCH 2/3] net/pcap: fix transmit return count in error conditions

[David Marchand]

When a packet cannot be transmitted, the driver is supposed to free this
packet and report it as handled.
This is to prevent the application from retrying to send the same packet
and ending up in a liveloop since the driver will never manage to send
it.

Fixes: 49a0a2ffd5db ("net/pcap: fix possible mbuf double freeing")
Fixes: 6db141c91e1f ("pcap: support jumbo frames")
CC: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 470867d..5e5aab7 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -354,7 +354,8 @@ struct pmd_devargs_all {
 					mbuf->pkt_len,
 					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
 
-				break;
+				rte_pktmbuf_free(mbuf);
+				continue;
 			}
 		}
 
@@ -373,7 +374,7 @@ struct pmd_devargs_all {
 	dumper_q->tx_stat.bytes += tx_bytes;
 	dumper_q->tx_stat.err_pkts += nb_pkts - num_tx;
 
-	return num_tx;
+	return nb_pkts;
 }
 
 /*
@@ -439,14 +440,15 @@ struct pmd_devargs_all {
 					mbuf->pkt_len,
 					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
 
-				break;
+				rte_pktmbuf_free(mbuf);
+				continue;
 			}
 		}
 
-		if (unlikely(ret != 0))
-			break;
-		num_tx++;
-		tx_bytes += mbuf->pkt_len;
+		if (ret == 0) {
+			num_tx++;
+			tx_bytes += mbuf->pkt_len;
+		}
 		rte_pktmbuf_free(mbuf);
 	}
 
@@ -454,7 +456,7 @@ struct pmd_devargs_all {
 	tx_queue->tx_stat.bytes += tx_bytes;
 	tx_queue->tx_stat.err_pkts += nb_pkts - num_tx;
 
-	return num_tx;
+	return nb_pkts;
 }
 
 /*
-- 
1.8.3.1

[dpdk-dev] [PATCH 3/3] net/pcap: fix concurrent multiseg packet transmits

[David Marchand]

Two cores can send multi segment packets on two different pcap ports.
Because of this, we can't have one single buffer to linearize packets.

Use rte_pktmbuf_read() to copy the packet into a buffer on the stack
and remove eth_pcap_gather_data().

Fixes: 6db141c91e1f ("pcap: support jumbo frames")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 90 +++++++++++++++--------------------------
 1 file changed, 32 insertions(+), 58 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 5e5aab7..7398b1b 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -46,7 +46,6 @@
 #define RTE_PMD_PCAP_MAX_QUEUES 16
 
 static char errbuf[PCAP_ERRBUF_SIZE];
-static unsigned char tx_pcap_data[RTE_ETH_PCAP_SNAPLEN];
 static struct timeval start_time;
 static uint64_t start_cycles;
 static uint64_t hz;
@@ -180,21 +179,6 @@ struct pmd_devargs_all {
 	return mbuf->nb_segs;
 }
 
-/* Copy data from mbuf chain to a buffer suitable for writing to a PCAP file. */
-static void
-eth_pcap_gather_data(unsigned char *data, struct rte_mbuf *mbuf)
-{
-	uint16_t data_len = 0;
-
-	while (mbuf) {
-		rte_memcpy(data + data_len, rte_pktmbuf_mtod(mbuf, void *),
-			mbuf->data_len);
-
-		data_len += mbuf->data_len;
-		mbuf = mbuf->next;
-	}
-}
-
 static uint16_t
 eth_pcap_rx_infinite(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 {
@@ -325,6 +309,9 @@ struct pmd_devargs_all {
 	uint32_t tx_bytes = 0;
 	struct pcap_pkthdr header;
 	pcap_dumper_t *dumper;
+	unsigned char _data[RTE_ETH_PCAP_SNAPLEN];
+	const unsigned char *data;
+	size_t len;
 
 	pp = rte_eth_devices[dumper_q->port_id].process_private;
 	dumper = pp->tx_dumper[dumper_q->queue_id];
@@ -336,31 +323,25 @@ struct pmd_devargs_all {
 	 * dumper */
 	for (i = 0; i < nb_pkts; i++) {
 		mbuf = bufs[i];
-		calculate_timestamp(&header.ts);
-		header.len = mbuf->pkt_len;
-		header.caplen = header.len;
-
-		if (likely(mbuf->nb_segs == 1)) {
-			pcap_dump((u_char *)dumper, &header,
-				  rte_pktmbuf_mtod(mbuf, void*));
+		len = rte_pktmbuf_pkt_len(mbuf);
+		if (likely(rte_pktmbuf_is_contiguous(mbuf))) {
+			data = rte_pktmbuf_mtod(mbuf, unsigned char *);
+		} else if (len <= sizeof(_data)) {
+			data = rte_pktmbuf_read(mbuf, 0, len, _data);
 		} else {
-			if (mbuf->pkt_len <= RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
-				eth_pcap_gather_data(tx_pcap_data, mbuf);
-				pcap_dump((u_char *)dumper, &header,
-					  tx_pcap_data);
-			} else {
-				PMD_LOG(ERR,
-					"Dropping PCAP packet. Size (%d) > max jumbo size (%d).",
-					mbuf->pkt_len,
-					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
-
-				rte_pktmbuf_free(mbuf);
-				continue;
-			}
+			PMD_LOG(ERR, "Dropping PCAP packet. Size (%zd) > max size (%zd).",
+				len, sizeof(_data));
+			rte_pktmbuf_free(mbuf);
+			continue;
 		}
 
+		calculate_timestamp(&header.ts);
+		header.len = len;
+		header.caplen = header.len;
+		pcap_dump((u_char *)dumper, &header, data);
+
 		num_tx++;
-		tx_bytes += mbuf->pkt_len;
+		tx_bytes += len;
 		rte_pktmbuf_free(mbuf);
 	}
 
@@ -408,13 +389,15 @@ struct pmd_devargs_all {
 eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 {
 	unsigned int i;
-	int ret;
 	struct rte_mbuf *mbuf;
 	struct pmd_process_private *pp;
 	struct pcap_tx_queue *tx_queue = queue;
 	uint16_t num_tx = 0;
 	uint32_t tx_bytes = 0;
 	pcap_t *pcap;
+	unsigned char _data[RTE_ETH_PCAP_SNAPLEN];
+	const unsigned char *data;
+	size_t len;
 
 	pp = rte_eth_devices[tx_queue->port_id].process_private;
 	pcap = pp->tx_pcap[tx_queue->queue_id];
@@ -424,30 +407,21 @@ struct pmd_devargs_all {
 
 	for (i = 0; i < nb_pkts; i++) {
 		mbuf = bufs[i];
-
-		if (likely(mbuf->nb_segs == 1)) {
-			ret = pcap_sendpacket(pcap,
-					rte_pktmbuf_mtod(mbuf, u_char *),
-					mbuf->pkt_len);
+		len = rte_pktmbuf_pkt_len(mbuf);
+		if (likely(rte_pktmbuf_is_contiguous(mbuf))) {
+			data = rte_pktmbuf_mtod(mbuf, unsigned char *);
+		} else if (len <= sizeof(_data)) {
+			data = rte_pktmbuf_read(mbuf, 0, len, _data);
 		} else {
-			if (mbuf->pkt_len <= RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
-				eth_pcap_gather_data(tx_pcap_data, mbuf);
-				ret = pcap_sendpacket(pcap,
-						tx_pcap_data, mbuf->pkt_len);
-			} else {
-				PMD_LOG(ERR,
-					"Dropping PCAP packet. Size (%d) > max jumbo size (%d).",
-					mbuf->pkt_len,
-					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
-
-				rte_pktmbuf_free(mbuf);
-				continue;
-			}
+			PMD_LOG(ERR, "Dropping PCAP packet. Size (%zd) > max size (%zd).",
+				len, sizeof(_data));
+			rte_pktmbuf_free(mbuf);
+			continue;
 		}
 
-		if (ret == 0) {
+		if (pcap_sendpacket(pcap, data, len) == 0) {
 			num_tx++;
-			tx_bytes += mbuf->pkt_len;
+			tx_bytes += len;
 		}
 		rte_pktmbuf_free(mbuf);
 	}
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 1/3] net/pcap: fix Rx with small buffers

[Ferruh Yigit]

On 7/24/2019 12:54 PM, David Marchand wrote:
> If the pkt pool contains only buffers smaller than the default headroom,
> then the driver will compute an invalid buffer size (negative value cast
> to an uint16_t).
> Rely on the mbuf api to check how much space is available in the mbuf.
> 
> Fixes: 6eb0ae218a98 ("pcap: fix mbuf allocation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH 2/3] net/pcap: fix transmit return count in error conditions

[Ferruh Yigit]

On 7/24/2019 12:54 PM, David Marchand wrote:
> When a packet cannot be transmitted, the driver is supposed to free this
> packet and report it as handled.
> This is to prevent the application from retrying to send the same packet
> and ending up in a liveloop since the driver will never manage to send
> it.
> 
> Fixes: 49a0a2ffd5db ("net/pcap: fix possible mbuf double freeing")
> Fixes: 6db141c91e1f ("pcap: support jumbo frames")
> CC: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/net/pcap/rte_eth_pcap.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index 470867d..5e5aab7 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -354,7 +354,8 @@ struct pmd_devargs_all {
>  					mbuf->pkt_len,
>  					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
>  
> -				break;
> +				rte_pktmbuf_free(mbuf);
> +				continue;

+1
Very recently 'rte_pktmbuf_free()' was moved because it wasn't compatible with
return value, but this looks better, to free the mbuf and record it as error.

>  			}
>  		}
>  
> @@ -373,7 +374,7 @@ struct pmd_devargs_all {
>  	dumper_q->tx_stat.bytes += tx_bytes;
>  	dumper_q->tx_stat.err_pkts += nb_pkts - num_tx;
>  
> -	return num_tx;
> +	return nb_pkts;
>  }
>  
>  /*
> @@ -439,14 +440,15 @@ struct pmd_devargs_all {
>  					mbuf->pkt_len,
>  					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
>  
> -				break;
> +				rte_pktmbuf_free(mbuf);
> +				continue;
>  			}
>  		}
>  
> -		if (unlikely(ret != 0))
> -			break;
> -		num_tx++;
> -		tx_bytes += mbuf->pkt_len;
> +		if (ret == 0) {
> +			num_tx++;
> +			tx_bytes += mbuf->pkt_len;
> +		}

I don't know this part, this is in 'eth_pcap_tx()' which writes packets to the
interfaces.

if 'pcap_sendpacket()' fails this doesn't mean packet can't be sent and may
cause a liveloop. Why not keep the existing behavior and let application to decide?

>  		rte_pktmbuf_free(mbuf);
>  	}
>  
> @@ -454,7 +456,7 @@ struct pmd_devargs_all {
>  	tx_queue->tx_stat.bytes += tx_bytes;
>  	tx_queue->tx_stat.err_pkts += nb_pkts - num_tx;
>  
> -	return num_tx;
> +	return nb_pkts;
>  }
>  
>  /*
> 

Re: [dpdk-dev] [PATCH 3/3] net/pcap: fix concurrent multiseg packet transmits

[Ferruh Yigit]

On 7/24/2019 12:54 PM, David Marchand wrote:
> Two cores can send multi segment packets on two different pcap ports.
> Because of this, we can't have one single buffer to linearize packets.

+1, you are right.

> 
> Use rte_pktmbuf_read() to copy the packet into a buffer on the stack
> and remove eth_pcap_gather_data().

+1

> 
> Fixes: 6db141c91e1f ("pcap: support jumbo frames")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH 2/3] net/pcap: fix transmit return count in error conditions

[David Marchand]

On Wed, Jul 24, 2019 at 8:36 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 7/24/2019 12:54 PM, David Marchand wrote:
> > When a packet cannot be transmitted, the driver is supposed to free this
> > packet and report it as handled.
> > This is to prevent the application from retrying to send the same packet
> > and ending up in a liveloop since the driver will never manage to send
> > it.
> >
> > Fixes: 49a0a2ffd5db ("net/pcap: fix possible mbuf double freeing")
> > Fixes: 6db141c91e1f ("pcap: support jumbo frames")
> > CC: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  drivers/net/pcap/rte_eth_pcap.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> > index 470867d..5e5aab7 100644
> > --- a/drivers/net/pcap/rte_eth_pcap.c
> > +++ b/drivers/net/pcap/rte_eth_pcap.c
> > @@ -354,7 +354,8 @@ struct pmd_devargs_all {
> >                                       mbuf->pkt_len,
> >                                       RTE_ETHER_MAX_JUMBO_FRAME_LEN);
> >
> > -                             break;
> > +                             rte_pktmbuf_free(mbuf);
> > +                             continue;
>
> +1
> Very recently 'rte_pktmbuf_free()' was moved because it wasn't compatible with
> return value, but this looks better, to free the mbuf and record it as error.
>
> >                       }
> >               }
> >
> > @@ -373,7 +374,7 @@ struct pmd_devargs_all {
> >       dumper_q->tx_stat.bytes += tx_bytes;
> >       dumper_q->tx_stat.err_pkts += nb_pkts - num_tx;
> >
> > -     return num_tx;
> > +     return nb_pkts;
> >  }
> >
> >  /*
> > @@ -439,14 +440,15 @@ struct pmd_devargs_all {
> >                                       mbuf->pkt_len,
> >                                       RTE_ETHER_MAX_JUMBO_FRAME_LEN);
> >
> > -                             break;
> > +                             rte_pktmbuf_free(mbuf);
> > +                             continue;
> >                       }
> >               }
> >
> > -             if (unlikely(ret != 0))
> > -                     break;
> > -             num_tx++;
> > -             tx_bytes += mbuf->pkt_len;
> > +             if (ret == 0) {
> > +                     num_tx++;
> > +                     tx_bytes += mbuf->pkt_len;
> > +             }
>
> I don't know this part, this is in 'eth_pcap_tx()' which writes packets to the
> interfaces.
>
> if 'pcap_sendpacket()' fails this doesn't mean packet can't be sent and may
> cause a liveloop. Why not keep the existing behavior and let application to decide?

The manual is not clear to me.
Do we really have temporary situations where retries are fine? and if
so, can we differentiate them from things like incorrect permissions
etc...

If we can't, dropping is safer.

-- 
David Marchand

Re: [dpdk-dev] [dpdk-stable] [PATCH 3/3] net/pcap: fix concurrent multiseg packet transmits

[David Marchand]

On Wed, Jul 24, 2019 at 1:55 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> Two cores can send multi segment packets on two different pcap ports.
> Because of this, we can't have one single buffer to linearize packets.
>
> Use rte_pktmbuf_read() to copy the packet into a buffer on the stack
> and remove eth_pcap_gather_data().
>
> Fixes: 6db141c91e1f ("pcap: support jumbo frames")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/net/pcap/rte_eth_pcap.c | 90 +++++++++++++++--------------------------
>  1 file changed, 32 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index 5e5aab7..7398b1b 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c

[snip]

> @@ -336,31 +323,25 @@ struct pmd_devargs_all {
>          * dumper */
>         for (i = 0; i < nb_pkts; i++) {
>                 mbuf = bufs[i];
> -               calculate_timestamp(&header.ts);
> -               header.len = mbuf->pkt_len;
> -               header.caplen = header.len;
> -
> -               if (likely(mbuf->nb_segs == 1)) {
> -                       pcap_dump((u_char *)dumper, &header,
> -                                 rte_pktmbuf_mtod(mbuf, void*));
> +               len = rte_pktmbuf_pkt_len(mbuf);
> +               if (likely(rte_pktmbuf_is_contiguous(mbuf))) {
> +                       data = rte_pktmbuf_mtod(mbuf, unsigned char *);
> +               } else if (len <= sizeof(_data)) {
> +                       data = rte_pktmbuf_read(mbuf, 0, len, _data);

We can actually skip the check on contiguous data, since
rte_pktmbuf_read returns a pointer to the mbuf data without copying.
WDYT ?


--
David Marchand

Re: [dpdk-dev] [PATCH 2/3] net/pcap: fix transmit return count in error conditions

[Ferruh Yigit]

On 7/25/2019 8:40 AM, David Marchand wrote:
> On Wed, Jul 24, 2019 at 8:36 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> On 7/24/2019 12:54 PM, David Marchand wrote:
>>> When a packet cannot be transmitted, the driver is supposed to free this
>>> packet and report it as handled.
>>> This is to prevent the application from retrying to send the same packet
>>> and ending up in a liveloop since the driver will never manage to send
>>> it.
>>>
>>> Fixes: 49a0a2ffd5db ("net/pcap: fix possible mbuf double freeing")
>>> Fixes: 6db141c91e1f ("pcap: support jumbo frames")
>>> CC: stable@dpdk.org
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> ---
>>>  drivers/net/pcap/rte_eth_pcap.c | 18 ++++++++++--------
>>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
>>> index 470867d..5e5aab7 100644
>>> --- a/drivers/net/pcap/rte_eth_pcap.c
>>> +++ b/drivers/net/pcap/rte_eth_pcap.c
>>> @@ -354,7 +354,8 @@ struct pmd_devargs_all {
>>>                                       mbuf->pkt_len,
>>>                                       RTE_ETHER_MAX_JUMBO_FRAME_LEN);
>>>
>>> -                             break;
>>> +                             rte_pktmbuf_free(mbuf);
>>> +                             continue;
>>
>> +1
>> Very recently 'rte_pktmbuf_free()' was moved because it wasn't compatible with
>> return value, but this looks better, to free the mbuf and record it as error.
>>
>>>                       }
>>>               }
>>>
>>> @@ -373,7 +374,7 @@ struct pmd_devargs_all {
>>>       dumper_q->tx_stat.bytes += tx_bytes;
>>>       dumper_q->tx_stat.err_pkts += nb_pkts - num_tx;
>>>
>>> -     return num_tx;
>>> +     return nb_pkts;
>>>  }
>>>
>>>  /*
>>> @@ -439,14 +440,15 @@ struct pmd_devargs_all {
>>>                                       mbuf->pkt_len,
>>>                                       RTE_ETHER_MAX_JUMBO_FRAME_LEN);
>>>
>>> -                             break;
>>> +                             rte_pktmbuf_free(mbuf);
>>> +                             continue;
>>>                       }
>>>               }
>>>
>>> -             if (unlikely(ret != 0))
>>> -                     break;
>>> -             num_tx++;
>>> -             tx_bytes += mbuf->pkt_len;
>>> +             if (ret == 0) {
>>> +                     num_tx++;
>>> +                     tx_bytes += mbuf->pkt_len;
>>> +             }
>>
>> I don't know this part, this is in 'eth_pcap_tx()' which writes packets to the
>> interfaces.
>>
>> if 'pcap_sendpacket()' fails this doesn't mean packet can't be sent and may
>> cause a liveloop. Why not keep the existing behavior and let application to decide?
> 
> The manual is not clear to me.
> Do we really have temporary situations where retries are fine?

Same here, I am not that clear, I assume these situations exists, specially
taking into account nobody complaining about existing implementation.

> and if
> so, can we differentiate them from things like incorrect permissions
> etc...
> 
> If we can't, dropping is safer.
> 

May not differentiate them, because all we are getting is the fail from API
without reason.

But overall instead of driving freeing a packet, it feels better to leave this
decision to application unless driver needs to free it.

Other changes in the patchset is fixing defects in driver, only this part
changes behavior without a clear defect, I am leaning to keep old behavior.

Re: [dpdk-dev] [dpdk-stable] [PATCH 3/3] net/pcap: fix concurrent multiseg packet transmits

[Ferruh Yigit]

On 7/25/2019 9:18 AM, David Marchand wrote:
> On Wed, Jul 24, 2019 at 1:55 PM David Marchand
> <david.marchand@redhat.com> wrote:
>>
>> Two cores can send multi segment packets on two different pcap ports.
>> Because of this, we can't have one single buffer to linearize packets.
>>
>> Use rte_pktmbuf_read() to copy the packet into a buffer on the stack
>> and remove eth_pcap_gather_data().
>>
>> Fixes: 6db141c91e1f ("pcap: support jumbo frames")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>> ---
>>  drivers/net/pcap/rte_eth_pcap.c | 90 +++++++++++++++--------------------------
>>  1 file changed, 32 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
>> index 5e5aab7..7398b1b 100644
>> --- a/drivers/net/pcap/rte_eth_pcap.c
>> +++ b/drivers/net/pcap/rte_eth_pcap.c
> 
> [snip]
> 
>> @@ -336,31 +323,25 @@ struct pmd_devargs_all {
>>          * dumper */
>>         for (i = 0; i < nb_pkts; i++) {
>>                 mbuf = bufs[i];
>> -               calculate_timestamp(&header.ts);
>> -               header.len = mbuf->pkt_len;
>> -               header.caplen = header.len;
>> -
>> -               if (likely(mbuf->nb_segs == 1)) {
>> -                       pcap_dump((u_char *)dumper, &header,
>> -                                 rte_pktmbuf_mtod(mbuf, void*));
>> +               len = rte_pktmbuf_pkt_len(mbuf);
>> +               if (likely(rte_pktmbuf_is_contiguous(mbuf))) {
>> +                       data = rte_pktmbuf_mtod(mbuf, unsigned char *);
>> +               } else if (len <= sizeof(_data)) {
>> +                       data = rte_pktmbuf_read(mbuf, 0, len, _data);
> 
> We can actually skip the check on contiguous data, since
> rte_pktmbuf_read returns a pointer to the mbuf data without copying.
> WDYT ?

Right, +1 to skip 'rte_pktmbuf_is_contiguous()' only it can be good to comment
'rte_pktmbuf_read()' usage to say it covers multi-segment too.

[dpdk-dev] [PATCH v2 0/3] Multiseg fixes for pcap pmd

[David Marchand]

Here are some fixes caught while looking at oerrors statistics for this
driver.
The second patch can be seen as a revert or a followup of [1].

1: https://git.dpdk.org/dpdk/commit/?id=49a0a2ffd5db

Changelog since v1:
- comments from Ferruh on patch2,
- little simplification in patch3,

-- 
David Marchand

David Marchand (3):
  net/pcap: fix Rx with small buffers
  net/pcap: fix transmit return count in error conditions
  net/pcap: fix concurrent multiseg packet transmits

 drivers/net/pcap/rte_eth_pcap.c | 104 ++++++++++++++++------------------------
 1 file changed, 40 insertions(+), 64 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH v2 1/3] net/pcap: fix Rx with small buffers

[David Marchand]

If the pkt pool contains only buffers smaller than the default headroom,
then the driver will compute an invalid buffer size (negative value cast
to an uint16_t).
Rely on the mbuf api to check how much space is available in the mbuf.

Fixes: 6eb0ae218a98 ("pcap: fix mbuf allocation")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 322c18f..470867d 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -242,7 +242,6 @@ eth_pcap_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	struct rte_mbuf *mbuf;
 	struct pcap_rx_queue *pcap_q = queue;
 	uint16_t num_rx = 0;
-	uint16_t buf_size;
 	uint32_t rx_bytes = 0;
 	pcap_t *pcap;
 
@@ -265,11 +264,7 @@ eth_pcap_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		if (unlikely(mbuf == NULL))
 			break;
 
-		/* Now get the space available for data in the mbuf */
-		buf_size = rte_pktmbuf_data_room_size(pcap_q->mb_pool) -
-				RTE_PKTMBUF_HEADROOM;
-
-		if (header.caplen <= buf_size) {
+		if (header.caplen <= rte_pktmbuf_tailroom(mbuf)) {
 			/* pcap packet will fit in the mbuf, can copy it */
 			rte_memcpy(rte_pktmbuf_mtod(mbuf, void *), packet,
 					header.caplen);
-- 
1.8.3.1

[dpdk-dev] [PATCH v2 2/3] net/pcap: fix transmit return count in error conditions

[David Marchand]

When a packet cannot be transmitted, the driver is supposed to free this
packet and report it as handled.
This is to prevent the application from retrying to send the same packet
and ending up in a liveloop since the driver will never manage to send
it.

Fixes: 49a0a2ffd5db ("net/pcap: fix possible mbuf double freeing")
Fixes: 6db141c91e1f ("pcap: support jumbo frames")
CC: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changelog since v1:
- restored the original behavior if pcap_sendpacket returns an error

---
 drivers/net/pcap/rte_eth_pcap.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 470867d..bfc0756 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -354,7 +354,8 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 					mbuf->pkt_len,
 					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
 
-				break;
+				rte_pktmbuf_free(mbuf);
+				continue;
 			}
 		}
 
@@ -373,7 +374,7 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	dumper_q->tx_stat.bytes += tx_bytes;
 	dumper_q->tx_stat.err_pkts += nb_pkts - num_tx;
 
-	return num_tx;
+	return nb_pkts;
 }
 
 /*
@@ -439,7 +440,8 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 					mbuf->pkt_len,
 					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
 
-				break;
+				rte_pktmbuf_free(mbuf);
+				continue;
 			}
 		}
 
@@ -452,9 +454,9 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 
 	tx_queue->tx_stat.pkts += num_tx;
 	tx_queue->tx_stat.bytes += tx_bytes;
-	tx_queue->tx_stat.err_pkts += nb_pkts - num_tx;
+	tx_queue->tx_stat.err_pkts += i - num_tx;
 
-	return num_tx;
+	return i;
 }
 
 /*
-- 
1.8.3.1

[dpdk-dev] [PATCH v2 3/3] net/pcap: fix concurrent multiseg packet transmits

[David Marchand]

Two cores can send multi segment packets on two different pcap ports.
Because of this, we can't have one single buffer to linearize packets.

Use rte_pktmbuf_read() to copy the packet into a buffer on the stack
and remove eth_pcap_gather_data() when necessary (if the mbuf is
contiguous, rte_pktmbuf_read() just points at the buffer address).

With this change, we won't support mono segment mbuf larger than 16k.

Fixes: 6db141c91e1f ("pcap: support jumbo frames")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changelog since v1:
- rely on rte_pktmbuf_read to handle both mono and multi segments cases

---
 drivers/net/pcap/rte_eth_pcap.c | 93 ++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 57 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index bfc0756..7a5fb6a 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -46,7 +46,6 @@
 #define RTE_PMD_PCAP_MAX_QUEUES 16
 
 static char errbuf[PCAP_ERRBUF_SIZE];
-static unsigned char tx_pcap_data[RTE_ETH_PCAP_SNAPLEN];
 static struct timeval start_time;
 static uint64_t start_cycles;
 static uint64_t hz;
@@ -180,21 +179,6 @@ eth_pcap_rx_jumbo(struct rte_mempool *mb_pool, struct rte_mbuf *mbuf,
 	return mbuf->nb_segs;
 }
 
-/* Copy data from mbuf chain to a buffer suitable for writing to a PCAP file. */
-static void
-eth_pcap_gather_data(unsigned char *data, struct rte_mbuf *mbuf)
-{
-	uint16_t data_len = 0;
-
-	while (mbuf) {
-		rte_memcpy(data + data_len, rte_pktmbuf_mtod(mbuf, void *),
-			mbuf->data_len);
-
-		data_len += mbuf->data_len;
-		mbuf = mbuf->next;
-	}
-}
-
 static uint16_t
 eth_pcap_rx_infinite(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 {
@@ -325,6 +309,8 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	uint32_t tx_bytes = 0;
 	struct pcap_pkthdr header;
 	pcap_dumper_t *dumper;
+	unsigned char temp_data[RTE_ETH_PCAP_SNAPLEN];
+	size_t len;
 
 	pp = rte_eth_devices[dumper_q->port_id].process_private;
 	dumper = pp->tx_dumper[dumper_q->queue_id];
@@ -336,31 +322,27 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	 * dumper */
 	for (i = 0; i < nb_pkts; i++) {
 		mbuf = bufs[i];
+		len = rte_pktmbuf_pkt_len(mbuf);
+		if (unlikely(!rte_pktmbuf_is_contiguous(mbuf) &&
+				len > sizeof(temp_data))) {
+			PMD_LOG(ERR, "Dropping multi segment PCAP packet. Size (%zd) > max size (%zd).",
+				len, sizeof(temp_data));
+			rte_pktmbuf_free(mbuf);
+			continue;
+		}
+
 		calculate_timestamp(&header.ts);
-		header.len = mbuf->pkt_len;
+		header.len = len;
 		header.caplen = header.len;
-
-		if (likely(mbuf->nb_segs == 1)) {
-			pcap_dump((u_char *)dumper, &header,
-				  rte_pktmbuf_mtod(mbuf, void*));
-		} else {
-			if (mbuf->pkt_len <= RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
-				eth_pcap_gather_data(tx_pcap_data, mbuf);
-				pcap_dump((u_char *)dumper, &header,
-					  tx_pcap_data);
-			} else {
-				PMD_LOG(ERR,
-					"Dropping PCAP packet. Size (%d) > max jumbo size (%d).",
-					mbuf->pkt_len,
-					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
-
-				rte_pktmbuf_free(mbuf);
-				continue;
-			}
-		}
+		/* rte_pktmbuf_read() returns a pointer to the data directly
+		 * in the mbuf (when the mbuf is contiguous) or, otherwise,
+		 * a pointer to temp_data after copying into it.
+		 */
+		pcap_dump((u_char *)dumper, &header,
+			  rte_pktmbuf_read(mbuf, 0, len, temp_data));
 
 		num_tx++;
-		tx_bytes += mbuf->pkt_len;
+		tx_bytes += len;
 		rte_pktmbuf_free(mbuf);
 	}
 
@@ -415,6 +397,8 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	uint16_t num_tx = 0;
 	uint32_t tx_bytes = 0;
 	pcap_t *pcap;
+	unsigned char temp_data[RTE_ETH_PCAP_SNAPLEN];
+	size_t len;
 
 	pp = rte_eth_devices[tx_queue->port_id].process_private;
 	pcap = pp->tx_pcap[tx_queue->queue_id];
@@ -424,31 +408,26 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 
 	for (i = 0; i < nb_pkts; i++) {
 		mbuf = bufs[i];
-
-		if (likely(mbuf->nb_segs == 1)) {
-			ret = pcap_sendpacket(pcap,
-					rte_pktmbuf_mtod(mbuf, u_char *),
-					mbuf->pkt_len);
-		} else {
-			if (mbuf->pkt_len <= RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
-				eth_pcap_gather_data(tx_pcap_data, mbuf);
-				ret = pcap_sendpacket(pcap,
-						tx_pcap_data, mbuf->pkt_len);
-			} else {
-				PMD_LOG(ERR,
-					"Dropping PCAP packet. Size (%d) > max jumbo size (%d).",
-					mbuf->pkt_len,
-					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
-
-				rte_pktmbuf_free(mbuf);
-				continue;
-			}
+		len = rte_pktmbuf_pkt_len(mbuf);
+		if (unlikely(!rte_pktmbuf_is_contiguous(mbuf) &&
+				len > sizeof(temp_data))) {
+			PMD_LOG(ERR, "Dropping multi segment PCAP packet. Size (%zd) > max size (%zd).",
+				len, sizeof(temp_data));
+			rte_pktmbuf_free(mbuf);
+			continue;
 		}
 
+		/* rte_pktmbuf_read() returns a pointer to the data directly
+		 * in the mbuf (when the mbuf is contiguous) or, otherwise,
+		 * a pointer to temp_data after copying into it.
+		 */
+		ret = pcap_sendpacket(pcap,
+				      rte_pktmbuf_read(mbuf, 0, len, temp_data),
+				      len);
 		if (unlikely(ret != 0))
 			break;
 		num_tx++;
-		tx_bytes += mbuf->pkt_len;
+		tx_bytes += len;
 		rte_pktmbuf_free(mbuf);
 	}
 
-- 
1.8.3.1

Re: [dpdk-dev] [dpdk-stable] [PATCH v2 3/3] net/pcap: fix concurrent multiseg packet transmits

[David Marchand]

On Thu, Jul 25, 2019 at 2:05 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> Two cores can send multi segment packets on two different pcap ports.
> Because of this, we can't have one single buffer to linearize packets.
>
> Use rte_pktmbuf_read() to copy the packet into a buffer on the stack
> and remove eth_pcap_gather_data() when necessary (if the mbuf is
> contiguous, rte_pktmbuf_read() just points at the buffer address).
>
> With this change, we won't support mono segment mbuf larger than 16k.

Argh, should have removed this wrong comment...


-- 
David Marchand

Re: [dpdk-dev] [PATCH v2 2/3] net/pcap: fix transmit return count in error conditions

[Ferruh Yigit]

On 7/25/2019 1:04 PM, David Marchand wrote:
> When a packet cannot be transmitted, the driver is supposed to free this
> packet and report it as handled.
> This is to prevent the application from retrying to send the same packet
> and ending up in a liveloop since the driver will never manage to send
> it.
> 
> Fixes: 49a0a2ffd5db ("net/pcap: fix possible mbuf double freeing")
> Fixes: 6db141c91e1f ("pcap: support jumbo frames")
> CC: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH v2 3/3] net/pcap: fix concurrent multiseg packet transmits

[Ferruh Yigit]

On 7/25/2019 1:04 PM, David Marchand wrote:
> Two cores can send multi segment packets on two different pcap ports.
> Because of this, we can't have one single buffer to linearize packets.
> 
> Use rte_pktmbuf_read() to copy the packet into a buffer on the stack
> and remove eth_pcap_gather_data() when necessary (if the mbuf is
> contiguous, rte_pktmbuf_read() just points at the buffer address).
> 
> With this change, we won't support mono segment mbuf larger than 16k.
> 
> Fixes: 6db141c91e1f ("pcap: support jumbo frames")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Thanks David, lgtm, only a few minor syntax issues, please check below.

<...>

> @@ -336,31 +322,27 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  	 * dumper */
>  	for (i = 0; i < nb_pkts; i++) {
>  		mbuf = bufs[i];
> +		len = rte_pktmbuf_pkt_len(mbuf);
> +		if (unlikely(!rte_pktmbuf_is_contiguous(mbuf) &&
> +				len > sizeof(temp_data))) {
> +			PMD_LOG(ERR, "Dropping multi segment PCAP packet. Size (%zd) > max size (%zd).",

Can we break the line to reduce the line length:
PMD_LOG(ERR,
	"Dropping multi segment PCAP packet. Size (%zd) > max size (%zd).",

> +				len, sizeof(temp_data));
> +			rte_pktmbuf_free(mbuf);
> +			continue;
> +		}
> +
>  		calculate_timestamp(&header.ts);
> -		header.len = mbuf->pkt_len;
> +		header.len = len;
>  		header.caplen = header.len;
> -
> -		if (likely(mbuf->nb_segs == 1)) {
> -			pcap_dump((u_char *)dumper, &header,
> -				  rte_pktmbuf_mtod(mbuf, void*));

DPDK coding convention requires a tab, instead of aligning to the parenthesis.

<...>

> +		/* rte_pktmbuf_read() returns a pointer to the data directly
> +		 * in the mbuf (when the mbuf is contiguous) or, otherwise,
> +		 * a pointer to temp_data after copying into it.
> +		 */
> +		pcap_dump((u_char *)dumper, &header,
> +			  rte_pktmbuf_read(mbuf, 0, len, temp_data));

Same here, not need to align to the parenthesis.

<...>

> +		len = rte_pktmbuf_pkt_len(mbuf);
> +		if (unlikely(!rte_pktmbuf_is_contiguous(mbuf) &&
> +				len > sizeof(temp_data))) {
> +			PMD_LOG(ERR, "Dropping multi segment PCAP packet. Size (%zd) > max size (%zd).",
> +				len, sizeof(temp_data));

ditto

> +			rte_pktmbuf_free(mbuf);
> +			continue;
>  		}
>  
> +		/* rte_pktmbuf_read() returns a pointer to the data directly
> +		 * in the mbuf (when the mbuf is contiguous) or, otherwise,
> +		 * a pointer to temp_data after copying into it.
> +		 */
> +		ret = pcap_sendpacket(pcap,
> +				      rte_pktmbuf_read(mbuf, 0, len, temp_data),
> +				      len);

ditto

[dpdk-dev] [PATCH v3 0/3] Multiseg fixes for pcap pmd

[David Marchand]

Here are some fixes caught while looking at oerrors statistics for this
driver.
The second patch can be seen as a revert or a followup of [1].

1: https://git.dpdk.org/dpdk/commit/?id=49a0a2ffd5db

Changelog since v2:
- coding style fixes in patch3,
- added acks,

Changelog since v1:
- comments from Ferruh on patch2,
- little simplification in patch3,

-- 
David Marchand

David Marchand (3):
  net/pcap: fix Rx with small buffers
  net/pcap: fix transmit return count in error conditions
  net/pcap: fix concurrent multiseg packet transmits

 drivers/net/pcap/rte_eth_pcap.c | 106 ++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 64 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH v3 1/3] net/pcap: fix Rx with small buffers

[David Marchand]

If the pkt pool contains only buffers smaller than the default headroom,
then the driver will compute an invalid buffer size (negative value cast
to an uint16_t).
Rely on the mbuf api to check how much space is available in the mbuf.

Fixes: 6eb0ae218a98 ("pcap: fix mbuf allocation")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 322c18f..470867d 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -242,7 +242,6 @@ eth_pcap_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	struct rte_mbuf *mbuf;
 	struct pcap_rx_queue *pcap_q = queue;
 	uint16_t num_rx = 0;
-	uint16_t buf_size;
 	uint32_t rx_bytes = 0;
 	pcap_t *pcap;
 
@@ -265,11 +264,7 @@ eth_pcap_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		if (unlikely(mbuf == NULL))
 			break;
 
-		/* Now get the space available for data in the mbuf */
-		buf_size = rte_pktmbuf_data_room_size(pcap_q->mb_pool) -
-				RTE_PKTMBUF_HEADROOM;
-
-		if (header.caplen <= buf_size) {
+		if (header.caplen <= rte_pktmbuf_tailroom(mbuf)) {
 			/* pcap packet will fit in the mbuf, can copy it */
 			rte_memcpy(rte_pktmbuf_mtod(mbuf, void *), packet,
 					header.caplen);
-- 
1.8.3.1

[dpdk-dev] [PATCH v3 2/3] net/pcap: fix transmit return count in error conditions

[David Marchand]

When a packet cannot be transmitted, the driver is supposed to free this
packet and report it as handled.
This is to prevent the application from retrying to send the same packet
and ending up in a liveloop since the driver will never manage to send
it.

Fixes: 49a0a2ffd5db ("net/pcap: fix possible mbuf double freeing")
Fixes: 6db141c91e1f ("pcap: support jumbo frames")
CC: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Changelog since v1:
- restored the original behavior if pcap_sendpacket returns an error

---
 drivers/net/pcap/rte_eth_pcap.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 470867d..bfc0756 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -354,7 +354,8 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 					mbuf->pkt_len,
 					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
 
-				break;
+				rte_pktmbuf_free(mbuf);
+				continue;
 			}
 		}
 
@@ -373,7 +374,7 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	dumper_q->tx_stat.bytes += tx_bytes;
 	dumper_q->tx_stat.err_pkts += nb_pkts - num_tx;
 
-	return num_tx;
+	return nb_pkts;
 }
 
 /*
@@ -439,7 +440,8 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 					mbuf->pkt_len,
 					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
 
-				break;
+				rte_pktmbuf_free(mbuf);
+				continue;
 			}
 		}
 
@@ -452,9 +454,9 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 
 	tx_queue->tx_stat.pkts += num_tx;
 	tx_queue->tx_stat.bytes += tx_bytes;
-	tx_queue->tx_stat.err_pkts += nb_pkts - num_tx;
+	tx_queue->tx_stat.err_pkts += i - num_tx;
 
-	return num_tx;
+	return i;
 }
 
 /*
-- 
1.8.3.1

[dpdk-dev] [PATCH v3 3/3] net/pcap: fix concurrent multiseg packet transmits

[David Marchand]

Two cores can send multi segment packets on two different pcap ports.
Because of this, we can't have one single buffer to linearize packets.

Use rte_pktmbuf_read() to copy the packet into a buffer on the stack
and remove eth_pcap_gather_data() when necessary (if the mbuf is
contiguous, rte_pktmbuf_read() just points at the buffer address).

Fixes: 6db141c91e1f ("pcap: support jumbo frames")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Changelog since v2:
- fixed commit log
- fixed coding style

Changelog since v1:
- rely on rte_pktmbuf_read to handle both mono and multi segments cases

---
 drivers/net/pcap/rte_eth_pcap.c | 95 +++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 57 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index bfc0756..da03b97 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -46,7 +46,6 @@
 #define RTE_PMD_PCAP_MAX_QUEUES 16
 
 static char errbuf[PCAP_ERRBUF_SIZE];
-static unsigned char tx_pcap_data[RTE_ETH_PCAP_SNAPLEN];
 static struct timeval start_time;
 static uint64_t start_cycles;
 static uint64_t hz;
@@ -180,21 +179,6 @@ eth_pcap_rx_jumbo(struct rte_mempool *mb_pool, struct rte_mbuf *mbuf,
 	return mbuf->nb_segs;
 }
 
-/* Copy data from mbuf chain to a buffer suitable for writing to a PCAP file. */
-static void
-eth_pcap_gather_data(unsigned char *data, struct rte_mbuf *mbuf)
-{
-	uint16_t data_len = 0;
-
-	while (mbuf) {
-		rte_memcpy(data + data_len, rte_pktmbuf_mtod(mbuf, void *),
-			mbuf->data_len);
-
-		data_len += mbuf->data_len;
-		mbuf = mbuf->next;
-	}
-}
-
 static uint16_t
 eth_pcap_rx_infinite(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 {
@@ -325,6 +309,8 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	uint32_t tx_bytes = 0;
 	struct pcap_pkthdr header;
 	pcap_dumper_t *dumper;
+	unsigned char temp_data[RTE_ETH_PCAP_SNAPLEN];
+	size_t len;
 
 	pp = rte_eth_devices[dumper_q->port_id].process_private;
 	dumper = pp->tx_dumper[dumper_q->queue_id];
@@ -336,31 +322,28 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	 * dumper */
 	for (i = 0; i < nb_pkts; i++) {
 		mbuf = bufs[i];
+		len = rte_pktmbuf_pkt_len(mbuf);
+		if (unlikely(!rte_pktmbuf_is_contiguous(mbuf) &&
+				len > sizeof(temp_data))) {
+			PMD_LOG(ERR,
+				"Dropping multi segment PCAP packet. Size (%zd) > max size (%zd).",
+				len, sizeof(temp_data));
+			rte_pktmbuf_free(mbuf);
+			continue;
+		}
+
 		calculate_timestamp(&header.ts);
-		header.len = mbuf->pkt_len;
+		header.len = len;
 		header.caplen = header.len;
-
-		if (likely(mbuf->nb_segs == 1)) {
-			pcap_dump((u_char *)dumper, &header,
-				  rte_pktmbuf_mtod(mbuf, void*));
-		} else {
-			if (mbuf->pkt_len <= RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
-				eth_pcap_gather_data(tx_pcap_data, mbuf);
-				pcap_dump((u_char *)dumper, &header,
-					  tx_pcap_data);
-			} else {
-				PMD_LOG(ERR,
-					"Dropping PCAP packet. Size (%d) > max jumbo size (%d).",
-					mbuf->pkt_len,
-					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
-
-				rte_pktmbuf_free(mbuf);
-				continue;
-			}
-		}
+		/* rte_pktmbuf_read() returns a pointer to the data directly
+		 * in the mbuf (when the mbuf is contiguous) or, otherwise,
+		 * a pointer to temp_data after copying into it.
+		 */
+		pcap_dump((u_char *)dumper, &header,
+				rte_pktmbuf_read(mbuf, 0, len, temp_data));
 
 		num_tx++;
-		tx_bytes += mbuf->pkt_len;
+		tx_bytes += len;
 		rte_pktmbuf_free(mbuf);
 	}
 
@@ -415,6 +398,8 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	uint16_t num_tx = 0;
 	uint32_t tx_bytes = 0;
 	pcap_t *pcap;
+	unsigned char temp_data[RTE_ETH_PCAP_SNAPLEN];
+	size_t len;
 
 	pp = rte_eth_devices[tx_queue->port_id].process_private;
 	pcap = pp->tx_pcap[tx_queue->queue_id];
@@ -424,31 +409,27 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 
 	for (i = 0; i < nb_pkts; i++) {
 		mbuf = bufs[i];
-
-		if (likely(mbuf->nb_segs == 1)) {
-			ret = pcap_sendpacket(pcap,
-					rte_pktmbuf_mtod(mbuf, u_char *),
-					mbuf->pkt_len);
-		} else {
-			if (mbuf->pkt_len <= RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
-				eth_pcap_gather_data(tx_pcap_data, mbuf);
-				ret = pcap_sendpacket(pcap,
-						tx_pcap_data, mbuf->pkt_len);
-			} else {
-				PMD_LOG(ERR,
-					"Dropping PCAP packet. Size (%d) > max jumbo size (%d).",
-					mbuf->pkt_len,
-					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
-
-				rte_pktmbuf_free(mbuf);
-				continue;
-			}
+		len = rte_pktmbuf_pkt_len(mbuf);
+		if (unlikely(!rte_pktmbuf_is_contiguous(mbuf) &&
+				len > sizeof(temp_data))) {
+			PMD_LOG(ERR,
+				"Dropping multi segment PCAP packet. Size (%zd) > max size (%zd).",
+				len, sizeof(temp_data));
+			rte_pktmbuf_free(mbuf);
+			continue;
 		}
 
+		/* rte_pktmbuf_read() returns a pointer to the data directly
+		 * in the mbuf (when the mbuf is contiguous) or, otherwise,
+		 * a pointer to temp_data after copying into it.
+		 */
+		ret = pcap_sendpacket(pcap,
+				rte_pktmbuf_read(mbuf, 0, len, temp_data),
+				len);
 		if (unlikely(ret != 0))
 			break;
 		num_tx++;
-		tx_bytes += mbuf->pkt_len;
+		tx_bytes += len;
 		rte_pktmbuf_free(mbuf);
 	}
 
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v3 0/3] Multiseg fixes for pcap pmd

[Ferruh Yigit]

On 7/25/2019 8:24 PM, David Marchand wrote:
> Here are some fixes caught while looking at oerrors statistics for this
> driver.
> The second patch can be seen as a revert or a followup of [1].
> 
> 1: https://git.dpdk.org/dpdk/commit/?id=49a0a2ffd5db
> 
> Changelog since v2:
> - coding style fixes in patch3,
> - added acks,
> 
> Changelog since v1:
> - comments from Ferruh on patch2,
> - little simplification in patch3,
> 

Series applied to dpdk-next-net/master, thanks.