-----Original Message-----
From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
Sent: Thursday, August 6, 2020 9:04 PM
To: wangyunjian <redacted>; dev@dpdk.org
Cc: keith.wiles@intel.com; ophirmu@mellanox.com; Lilijun (Jerry)
[off-list ref]; xudingke [off-list ref];
stable@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] net/tap: free mempool when closing

On 8/6/2020 1:45 PM, wangyunjian wrote:

quoted

quoted

-----Original Message-----
From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
Sent: Thursday, August 6, 2020 12:36 AM
To: wangyunjian <redacted>; dev@dpdk.org
Cc: keith.wiles@intel.com; ophirmu@mellanox.com; Lilijun (Jerry)
[off-list ref]; xudingke [off-list ref];
stable@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] net/tap: free mempool when closing

On 7/29/2020 12:35 PM, wangyunjian wrote:

quoted

From: Yunjian Wang <redacted>

When setup tx queues, we will create a mempool for the 'gso_ctx'.
The mempool is not freed when closing tap device. If free the tap
device and create it with different name, it will create a new
mempool. This maybe cause an OOM.

Fixes: 050316a88313 ("net/tap: support TSO (TCP Segment Offload)")
Cc: stable@dpdk.org

Signed-off-by: Yunjian Wang <redacted>

<...>

quoted

@@ -1317,26 +1320,31 @@ tap_gso_ctx_setup(struct rte_gso_ctx

*gso_ctx,

struct rte_eth_dev *dev)

quoted

 {
 	uint32_t gso_types;
 	char pool_name[64];
-
-	/*
-	 * Create private mbuf pool with TAP_GSO_MBUF_SEG_SIZE bytes
-	 * size per mbuf use this pool for both direct and indirect mbufs
-	 */
-
-	struct rte_mempool *mp;      /* Mempool for GSO packets */
+	struct pmd_internals *pmd = dev->data->dev_private;
+	int ret;

 	/* initialize GSO context */
 	gso_types = DEV_TX_OFFLOAD_TCP_TSO;
-	snprintf(pool_name, sizeof(pool_name), "mp_%s",

dev->device->name);

quoted

quoted

quoted

-	mp = rte_mempool_lookup((const char *)pool_name);
-	if (!mp) {
-		mp = rte_pktmbuf_pool_create(pool_name,

TAP_GSO_MBUFS_NUM,

quoted

quoted

quoted

-			TAP_GSO_MBUF_CACHE_SIZE, 0,
+	if (!pmd->gso_ctx_mp) {
+		/*
+		 * Create private mbuf pool with TAP_GSO_MBUF_SEG_SIZE
+		 * bytes size per mbuf use this pool for both direct and
+		 * indirect mbufs
+		 */
+		ret = snprintf(pool_name, sizeof(pool_name), "mp_%s",
+				dev->device->name);
+		if (ret < 0 || ret >= (int)sizeof(pool_name)) {
+			TAP_LOG(ERR,
+				"%s: failed to create mbuf pool "
+				"name for device %s\n",
+				pmd->name, dev->device->name);

Overall looks good. Only above error doesn't say why it failed,
informing the user that device name is too long may help her to overcome

the error.

quoted

I found that the return value of functions snprintf was not checked
when modifying the code, so fixed it.
I think it maybe fail, because the max device name length is
RTE_DEV_NAME_MAX_LEN(64).

+1 to the check.
My comment was on the log message, which says "failed to create mbuf pool",
but it doesn't say it is failed because of long device name.
If user knows the reason of the failure, can prevent it by providing shorter
device name.
My suggestion is update the error log message to have the reason of failure.

Thanks for your suggestion, will include them in next version.

quoted

Do I need to split into two patches?

I think OK to have the change in this patch.

OK

quoted

Thanks,
Yunjian