[dpdk-dev] [PATCH] net/pcap: fix issue with unnecessary mbufs freeing

[dpdk-dev] [PATCH] net/pcap: fix issue with unnecessary mbufs freeing

[A.McLoughlin]

In the eth_pcap_tx() and eth_pcap_tx_dumper() functions mbufs were freed
without incrementing num_tx. To fix the issue, the mbuf freeing was
removed as it was not of any benefit.

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

Signed-off-by: A.McLoughlin <aideen.mcloughlin@intel.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 26e85183e..77bb66337 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -349,7 +349,6 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 					mbuf->pkt_len,
 					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
 
-				rte_pktmbuf_free(mbuf);
 				break;
 			}
 		}
@@ -435,7 +434,6 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 					mbuf->pkt_len,
 					RTE_ETHER_MAX_JUMBO_FRAME_LEN);
 
-				rte_pktmbuf_free(mbuf);
 				break;
 			}
 		}
-- 
2.17.1

--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

Re: [dpdk-dev] [PATCH] net/pcap: fix issue with unnecessary mbufs freeing

[Ferruh Yigit]

On 7/11/2019 2:59 PM, A.McLoughlin wrote:
> In the eth_pcap_tx() and eth_pcap_tx_dumper() functions mbufs were freed
> without incrementing num_tx. To fix the issue, the mbuf freeing was
> removed as it was not of any benefit.
> 
> Fixes: 6db141c91e1f ("pcap: support jumbo frames")
> Cc: stable@dpdk.org
> Cc: tero.aho@coriant.com
> 
> Signed-off-by: A.McLoughlin <aideen.mcloughlin@intel.com>

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

(It may be good to mention PMD was freeing the mbuf without notifying the app,
which may lead double freeing of the mbuf (crash) or app using invalid mbuf.
I will add while merging.)

Re: [dpdk-dev] [PATCH] net/pcap: fix issue with unnecessary mbufs freeing

[Ferruh Yigit]

On 7/11/2019 3:15 PM, Ferruh Yigit wrote:
> On 7/11/2019 2:59 PM, A.McLoughlin wrote:
>> In the eth_pcap_tx() and eth_pcap_tx_dumper() functions mbufs were freed
>> without incrementing num_tx. To fix the issue, the mbuf freeing was
>> removed as it was not of any benefit.
>>
>> Fixes: 6db141c91e1f ("pcap: support jumbo frames")
>> Cc: stable@dpdk.org
>> Cc: tero.aho@coriant.com
>>
>> Signed-off-by: A.McLoughlin <aideen.mcloughlin@intel.com>
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> (It may be good to mention PMD was freeing the mbuf without notifying the app,
> which may lead double freeing of the mbuf (crash) or app using invalid mbuf.
> I will add while merging.)
> 

Applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [dpdk-stable] [PATCH] net/pcap: fix issue with unnecessary mbufs freeing

[David Marchand]

On Thu, Jul 11, 2019 at 4:00 PM A.McLoughlin
<aideen.mcloughlin@intel.com> wrote:
>
> In the eth_pcap_tx() and eth_pcap_tx_dumper() functions mbufs were freed
> without incrementing num_tx. To fix the issue, the mbuf freeing was
> removed as it was not of any benefit.
>
> Fixes: 6db141c91e1f ("pcap: support jumbo frames")
> Cc: stable@dpdk.org
> Cc: tero.aho@coriant.com
>
> Signed-off-by: A.McLoughlin <aideen.mcloughlin@intel.com>
> ---
>  drivers/net/pcap/rte_eth_pcap.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index 26e85183e..77bb66337 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -349,7 +349,6 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>                                         mbuf->pkt_len,
>                                         RTE_ETHER_MAX_JUMBO_FRAME_LEN);
>
> -                               rte_pktmbuf_free(mbuf);
>                                 break;
>                         }
>                 }
> @@ -435,7 +434,6 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>                                         mbuf->pkt_len,
>                                         RTE_ETHER_MAX_JUMBO_FRAME_LEN);
>
> -                               rte_pktmbuf_free(mbuf);
>                                 break;
>                         }
>                 }
> --
> 2.17.1
>

If a driver cannot xmit a packet (it is not a temporary situation but
it is just that it can't), then it must free it and report it as
handled because the application can do nothing more.
Imagine an application that retries to send the packet right away, it
ends up in a liveloop.

The freeing of the packet was correct, what needs to be fixed is the
return value.
I am preparing fixes (found another issue in this part of the driver).
I would be for reverting this patch, so that the fix is more
straightforward, but we can discuss this once I sent my patches.


> --------------------------------------------------------------
> Intel Research and Development Ireland Limited
> Registered in Ireland
> Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
> Registered Number: 308263
>
>
> This e-mail and any attachments may contain confidential material for the sole
> use of the intended recipient(s). Any review or distribution by others is
> strictly prohibited. If you are not the intended recipient, please contact the
> sender and delete all copies.
>


Please contact your IT and do something about this footer.



--
David Marchand