From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 30ADCA0540; Mon, 4 Jul 2022 15:44:14 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C2941410E5; Mon, 4 Jul 2022 15:44:13 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mails.dpdk.org (Postfix) with ESMTP id D3A0640E09; Mon, 4 Jul 2022 15:44:11 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1656942252; x=1688478252; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=F/qOL5zxPnOg4xT4vxKsHhfs0pCdwlGiiTaooKyGeFI=; b=fTVTetKhvTul1r/fpDULw8AbS1bMRIXfGCj1Y3jOcFSZaO9/a6e0rkHr QDtXgZcn5/ttJumqQnZYk39DbglOPsdyNRoSGXF0hZEsryn9hfavQ3xdo PxnARUa0ssR5dXZBPKn3LQ6a1BhZA0I+HlOarr1oVEJn0R18KvM+IXWSh c2jiJxGDG5TD9ByvAeSFx1BUgRwJZUOhTq8VEYZqmmulywCpXLTtvhHmy 3yJrwjq24lvkf+L2XZB1WUUVGwynwboKyc+LstsEaS2JQaqAwLsy1IEI/ C4/xtnV6x13Gmaa/g6Jwix3+lkC+XDGUULIKwJ6EyW1pSXJfaxyfurY7T A==; X-IronPort-AV: E=McAfee;i="6400,9594,10397"; a="266157207" X-IronPort-AV: E=Sophos;i="5.92,243,1650956400"; d="scan'208";a="266157207" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Jul 2022 06:44:10 -0700 X-IronPort-AV: E=Sophos;i="5.92,243,1650956400"; d="scan'208";a="619298121" Received: from bricha3-mobl.ger.corp.intel.com ([10.55.133.37]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 04 Jul 2022 06:44:09 -0700 Date: Mon, 4 Jul 2022 14:44:06 +0100 From: Bruce Richardson To: Kevin Laatz Cc: dev@dpdk.org, stable@dpdk.org, Xingguang He Subject: Re: [PATCH v2 1/3] dma/idxd: fix memory leak in pci close Message-ID: References: <20220408141504.1319913-1-kevin.laatz@intel.com> <20220703122243.929642-1-kevin.laatz@intel.com> <20220703122243.929642-2-kevin.laatz@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Mon, Jul 04, 2022 at 02:34:31PM +0100, Kevin Laatz wrote: > On 04/07/2022 14:19, Bruce Richardson wrote: > > On Sun, Jul 03, 2022 at 01:22:41PM +0100, Kevin Laatz wrote: > > > ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev' > > > struct. > > > > > > This is fixed by free'ing the struct when the last queue on the PCI > > > device is being closed. > > > > > > Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe") > > > Cc: stable@dpdk.org > > > Cc: bruce.richardson@intel.com > > > > > > Reported-by: Xingguang He > > > Signed-off-by: Kevin Laatz > > > --- > > > drivers/dma/idxd/idxd_common.c | 2 ++ > > > drivers/dma/idxd/idxd_internal.h | 2 ++ > > > drivers/dma/idxd/idxd_pci.c | 34 +++++++++++++++++++++++++------- > > > 3 files changed, 31 insertions(+), 7 deletions(-) > > > > > Some comments inline below. > > > > /Bruce > > > > > diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c > > > index c77200a457..d347bbed21 100644 > > > --- a/drivers/dma/idxd/idxd_common.c > > > +++ b/drivers/dma/idxd/idxd_common.c > > > @@ -620,6 +620,8 @@ idxd_dmadev_create(const char *name, struct rte_device *dev, > > > dmadev->fp_obj->dev_private = idxd; > > > idxd->dmadev->state = RTE_DMA_DEV_READY; > > > + if (idxd->u.pci != NULL) > > > + rte_atomic16_inc(&idxd->u.pci->ref_count); > > > return 0; > > I don't think this belongs in the common code. Can it be put somewhere in > > the pci-specific driver code to avoid issues, e.g. after idxd_dmadev_create > > returns in probe_pci() function. > > Sure, I'll look to move it in v3. > > [snip] > > @@ -159,12 +178,13 @@ init_pci_device(struct rte_pci_device *dev, struct idxd_dmadev *idxd, > > uint8_t lg2_max_batch, lg2_max_copy_size; > > unsigned int i, err_code; > > - pci = malloc(sizeof(*pci)); > > + pci = rte_malloc(NULL, sizeof(*pci), 0); > > Any particular reason for the change from regular malloc to rte_malloc? > > Mainly consistency, its the only place in the driver where malloc is used > instead of rte_malloc. > > I have no strong preference here - I can remove the change for v3. > If it's inconsistent with the rest, please do keep the change in v3.