* [PATCH 0/3] fix some problems of flower firmware @ 2023-12-14 10:24 Chaoyong He 2023-12-14 10:24 ` [PATCH 1/3] net/nfp: fix close representor problem Chaoyong He ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Chaoyong He @ 2023-12-14 10:24 UTC (permalink / raw) To: dev; +Cc: oss-drivers, Chaoyong He This patch series fix some problems of flower firmware. Long Wu (3): net/nfp: fix close representor problem net/nfp: fix free resource problem net/nfp: free domain ID in close interface drivers/net/nfp/flower/nfp_flower.c | 1 + .../net/nfp/flower/nfp_flower_representor.c | 21 +++++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) -- 2.39.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/3] net/nfp: fix close representor problem 2023-12-14 10:24 [PATCH 0/3] fix some problems of flower firmware Chaoyong He @ 2023-12-14 10:24 ` Chaoyong He 2023-12-14 10:24 ` [PATCH 2/3] net/nfp: fix free resource problem Chaoyong He ` (2 subsequent siblings) 3 siblings, 0 replies; 14+ messages in thread From: Chaoyong He @ 2023-12-14 10:24 UTC (permalink / raw) To: dev; +Cc: oss-drivers, Long Wu, chaoyong.he, stable, Peng Zhang From: Long Wu <long.wu@corigine.com> Close interface use "rte_eth_dev_destroy()" to destroy representor. The "rte_eth_dev_destroy()" will call "rte_eth_dev_release_port()" but the "rte_eth_dev_close()" also calls "rte_eth_dev_release_port()". This will cause Segmentation fault. Remove the "rte_eth_dev_destroy()" in nfp representor close interface. Fixes: a135bc1644d6 ("net/nfp: fix resource leak for flower firmware") Cc: chaoyong.he@corigine.com Cc: stable@dpdk.org Signed-off-by: Long Wu <long.wu@corigine.com> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com> Reviewed-by: Peng Zhang <peng.zhang@corigine.com> --- drivers/net/nfp/flower/nfp_flower_representor.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c b/drivers/net/nfp/flower/nfp_flower_representor.c index 7d8c055b80..27ea3891bd 100644 --- a/drivers/net/nfp/flower/nfp_flower_representor.c +++ b/drivers/net/nfp/flower/nfp_flower_representor.c @@ -314,13 +314,13 @@ nfp_flower_repr_free(struct nfp_flower_representor *repr, { switch (repr_type) { case NFP_REPR_TYPE_PHYS_PORT: - rte_eth_dev_destroy(repr->eth_dev, nfp_flower_repr_uninit); + nfp_flower_repr_uninit(repr->eth_dev); break; case NFP_REPR_TYPE_PF: - rte_eth_dev_destroy(repr->eth_dev, nfp_flower_pf_repr_uninit); + nfp_flower_pf_repr_uninit(repr->eth_dev); break; case NFP_REPR_TYPE_VF: - rte_eth_dev_destroy(repr->eth_dev, nfp_flower_repr_uninit); + nfp_flower_repr_uninit(repr->eth_dev); break; default: PMD_DRV_LOG(ERR, "Unsupported repr port type."); -- 2.39.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] net/nfp: fix free resource problem 2023-12-14 10:24 [PATCH 0/3] fix some problems of flower firmware Chaoyong He 2023-12-14 10:24 ` [PATCH 1/3] net/nfp: fix close representor problem Chaoyong He @ 2023-12-14 10:24 ` Chaoyong He 2023-12-15 12:54 ` Ferruh Yigit 2023-12-14 10:24 ` [PATCH 3/3] net/nfp: free domain ID in close interface Chaoyong He 2024-01-12 12:05 ` [PATCH 0/3] fix some problems of flower firmware Ferruh Yigit 3 siblings, 1 reply; 14+ messages in thread From: Chaoyong He @ 2023-12-14 10:24 UTC (permalink / raw) To: dev; +Cc: oss-drivers, Long Wu, chaoyong.he, stable, Peng Zhang From: Long Wu <long.wu@corigine.com> Set the representor array to NULL to avoid that close interface does not free some resource. Fixes: a135bc1644d6 ("net/nfp: fix resource leak for flower firmware") Cc: chaoyong.he@corigine.com Cc: stable@dpdk.org Signed-off-by: Long Wu <long.wu@corigine.com> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com> Reviewed-by: Peng Zhang <peng.zhang@corigine.com> --- drivers/net/nfp/flower/nfp_flower_representor.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c b/drivers/net/nfp/flower/nfp_flower_representor.c index 27ea3891bd..5f7c1fa737 100644 --- a/drivers/net/nfp/flower/nfp_flower_representor.c +++ b/drivers/net/nfp/flower/nfp_flower_representor.c @@ -294,17 +294,30 @@ nfp_flower_repr_tx_burst(void *tx_queue, static int nfp_flower_repr_uninit(struct rte_eth_dev *eth_dev) { + uint16_t index; struct nfp_flower_representor *repr; repr = eth_dev->data->dev_private; rte_ring_free(repr->ring); + if (repr->repr_type == NFP_REPR_TYPE_PHYS_PORT) { + index = NFP_FLOWER_CMSG_PORT_PHYS_PORT_NUM(repr->port_id); + repr->app_fw_flower->phy_reprs[index] = NULL; + } else { + index = repr->vf_id; + repr->app_fw_flower->vf_reprs[index] = NULL; + } + return 0; } static int -nfp_flower_pf_repr_uninit(__rte_unused struct rte_eth_dev *eth_dev) +nfp_flower_pf_repr_uninit(struct rte_eth_dev *eth_dev) { + struct nfp_flower_representor *repr = eth_dev->data->dev_private; + + repr->app_fw_flower->pf_repr = NULL; + return 0; } -- 2.39.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] net/nfp: fix free resource problem 2023-12-14 10:24 ` [PATCH 2/3] net/nfp: fix free resource problem Chaoyong He @ 2023-12-15 12:54 ` Ferruh Yigit 2023-12-18 1:50 ` Chaoyong He 0 siblings, 1 reply; 14+ messages in thread From: Ferruh Yigit @ 2023-12-15 12:54 UTC (permalink / raw) To: Chaoyong He, dev; +Cc: oss-drivers, Long Wu, stable, Peng Zhang On 12/14/2023 10:24 AM, Chaoyong He wrote: > From: Long Wu <long.wu@corigine.com> > > Set the representor array to NULL to avoid that close interface > does not free some resource. > > Fixes: a135bc1644d6 ("net/nfp: fix resource leak for flower firmware") > Cc: chaoyong.he@corigine.com > Cc: stable@dpdk.org > > Signed-off-by: Long Wu <long.wu@corigine.com> > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com> > Reviewed-by: Peng Zhang <peng.zhang@corigine.com> > --- > drivers/net/nfp/flower/nfp_flower_representor.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c b/drivers/net/nfp/flower/nfp_flower_representor.c > index 27ea3891bd..5f7c1fa737 100644 > --- a/drivers/net/nfp/flower/nfp_flower_representor.c > +++ b/drivers/net/nfp/flower/nfp_flower_representor.c > @@ -294,17 +294,30 @@ nfp_flower_repr_tx_burst(void *tx_queue, > static int > nfp_flower_repr_uninit(struct rte_eth_dev *eth_dev) > { > + uint16_t index; > struct nfp_flower_representor *repr; > > repr = eth_dev->data->dev_private; > rte_ring_free(repr->ring); > > + if (repr->repr_type == NFP_REPR_TYPE_PHYS_PORT) { > + index = NFP_FLOWER_CMSG_PORT_PHYS_PORT_NUM(repr->port_id); > + repr->app_fw_flower->phy_reprs[index] = NULL; > + } else { > + index = repr->vf_id; > + repr->app_fw_flower->vf_reprs[index] = NULL; > + } > + > return 0; > } > > static int > -nfp_flower_pf_repr_uninit(__rte_unused struct rte_eth_dev *eth_dev) > +nfp_flower_pf_repr_uninit(struct rte_eth_dev *eth_dev) > { > + struct nfp_flower_representor *repr = eth_dev->data->dev_private; > + > + repr->app_fw_flower->pf_repr = NULL; > Here it is assigned to NULL but is it freed? If freed, why not set to NULL where it is freed? Same for above phy_reprs & vf_reprs. ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 2/3] net/nfp: fix free resource problem 2023-12-15 12:54 ` Ferruh Yigit @ 2023-12-18 1:50 ` Chaoyong He 2024-01-08 15:42 ` Ferruh Yigit 0 siblings, 1 reply; 14+ messages in thread From: Chaoyong He @ 2023-12-18 1:50 UTC (permalink / raw) To: Ferruh Yigit, dev; +Cc: oss-drivers, Long Wu, stable, Nole Zhang > On 12/14/2023 10:24 AM, Chaoyong He wrote: > > From: Long Wu <long.wu@corigine.com> > > > > Set the representor array to NULL to avoid that close interface does > > not free some resource. > > > > Fixes: a135bc1644d6 ("net/nfp: fix resource leak for flower firmware") > > Cc: chaoyong.he@corigine.com > > Cc: stable@dpdk.org > > > > Signed-off-by: Long Wu <long.wu@corigine.com> > > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com> > > Reviewed-by: Peng Zhang <peng.zhang@corigine.com> > > --- > > drivers/net/nfp/flower/nfp_flower_representor.c | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c > > b/drivers/net/nfp/flower/nfp_flower_representor.c > > index 27ea3891bd..5f7c1fa737 100644 > > --- a/drivers/net/nfp/flower/nfp_flower_representor.c > > +++ b/drivers/net/nfp/flower/nfp_flower_representor.c > > @@ -294,17 +294,30 @@ nfp_flower_repr_tx_burst(void *tx_queue, static > > int nfp_flower_repr_uninit(struct rte_eth_dev *eth_dev) { > > + uint16_t index; > > struct nfp_flower_representor *repr; > > > > repr = eth_dev->data->dev_private; > > rte_ring_free(repr->ring); > > > > + if (repr->repr_type == NFP_REPR_TYPE_PHYS_PORT) { > > + index = NFP_FLOWER_CMSG_PORT_PHYS_PORT_NUM(repr- > >port_id); > > + repr->app_fw_flower->phy_reprs[index] = NULL; > > + } else { > > + index = repr->vf_id; > > + repr->app_fw_flower->vf_reprs[index] = NULL; > > + } > > + > > return 0; > > } > > > > static int > > -nfp_flower_pf_repr_uninit(__rte_unused struct rte_eth_dev *eth_dev) > > +nfp_flower_pf_repr_uninit(struct rte_eth_dev *eth_dev) > > { > > + struct nfp_flower_representor *repr = eth_dev->data->dev_private; > > + > > + repr->app_fw_flower->pf_repr = NULL; > > > > Here it is assigned to NULL but is it freed? If freed, why not set to NULL where > it is freed? > > Same for above phy_reprs & vf_reprs. The whole invoke view: rte_eth_dev_close() --> nfp_flower_repr_dev_close() --> nfp_flower_repr_free() --> nfp_flower_pf_repr_uninit() --> nfp_flower_repr_uninit() // In these two functions, we just assigned to NULL but not freed yet. // It is still refer by the `eth_dev->data->dev_private`. --> rte_eth_dev_release_port() --> rte_free(eth_dev->data->dev_private); // And here it is really freed (by the rte framework). ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] net/nfp: fix free resource problem 2023-12-18 1:50 ` Chaoyong He @ 2024-01-08 15:42 ` Ferruh Yigit 2024-01-09 7:56 ` Chaoyong He 0 siblings, 1 reply; 14+ messages in thread From: Ferruh Yigit @ 2024-01-08 15:42 UTC (permalink / raw) To: Chaoyong He, dev; +Cc: oss-drivers, Long Wu, stable, Nole Zhang On 12/18/2023 1:50 AM, Chaoyong He wrote: >> On 12/14/2023 10:24 AM, Chaoyong He wrote: >>> From: Long Wu <long.wu@corigine.com> >>> >>> Set the representor array to NULL to avoid that close interface does >>> not free some resource. >>> >>> Fixes: a135bc1644d6 ("net/nfp: fix resource leak for flower firmware") >>> Cc: chaoyong.he@corigine.com >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Long Wu <long.wu@corigine.com> >>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com> >>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com> >>> --- >>> drivers/net/nfp/flower/nfp_flower_representor.c | 15 ++++++++++++++- >>> 1 file changed, 14 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c >>> b/drivers/net/nfp/flower/nfp_flower_representor.c >>> index 27ea3891bd..5f7c1fa737 100644 >>> --- a/drivers/net/nfp/flower/nfp_flower_representor.c >>> +++ b/drivers/net/nfp/flower/nfp_flower_representor.c >>> @@ -294,17 +294,30 @@ nfp_flower_repr_tx_burst(void *tx_queue, static >>> int nfp_flower_repr_uninit(struct rte_eth_dev *eth_dev) { >>> + uint16_t index; >>> struct nfp_flower_representor *repr; >>> >>> repr = eth_dev->data->dev_private; >>> rte_ring_free(repr->ring); >>> >>> + if (repr->repr_type == NFP_REPR_TYPE_PHYS_PORT) { >>> + index = NFP_FLOWER_CMSG_PORT_PHYS_PORT_NUM(repr- >>> port_id); >>> + repr->app_fw_flower->phy_reprs[index] = NULL; >>> + } else { >>> + index = repr->vf_id; >>> + repr->app_fw_flower->vf_reprs[index] = NULL; >>> + } >>> + >>> return 0; >>> } >>> >>> static int >>> -nfp_flower_pf_repr_uninit(__rte_unused struct rte_eth_dev *eth_dev) >>> +nfp_flower_pf_repr_uninit(struct rte_eth_dev *eth_dev) >>> { >>> + struct nfp_flower_representor *repr = eth_dev->data->dev_private; >>> + >>> + repr->app_fw_flower->pf_repr = NULL; >>> >> >> Here it is assigned to NULL but is it freed? If freed, why not set to NULL where >> it is freed? >> >> Same for above phy_reprs & vf_reprs. > > The whole invoke view: > rte_eth_dev_close() > --> nfp_flower_repr_dev_close() > --> nfp_flower_repr_free() > --> nfp_flower_pf_repr_uninit() > --> nfp_flower_repr_uninit() > // In these two functions, we just assigned to NULL but not freed yet. > // It is still refer by the `eth_dev->data->dev_private`. > --> rte_eth_dev_release_port() > --> rte_free(eth_dev->data->dev_private); > // And here it is really freed (by the rte framework). > 'rte_eth_dev_release_port()' frees the device private data, but not all pointers, like 'repr->app_fw_flower->pf_repr', in the struct are freed, it is dev_close() or unint() functions responsibility. Can you please double check if 'eth_dev->data->dev_private->app_fw_flower->pf_repr' freed or not? ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 2/3] net/nfp: fix free resource problem 2024-01-08 15:42 ` Ferruh Yigit @ 2024-01-09 7:56 ` Chaoyong He 2024-01-09 17:48 ` Ferruh Yigit 0 siblings, 1 reply; 14+ messages in thread From: Chaoyong He @ 2024-01-09 7:56 UTC (permalink / raw) To: Ferruh Yigit, dev; +Cc: oss-drivers, Long Wu, stable, Nole Zhang > On 12/18/2023 1:50 AM, Chaoyong He wrote: > >> On 12/14/2023 10:24 AM, Chaoyong He wrote: > >>> From: Long Wu <long.wu@corigine.com> > >>> > >>> Set the representor array to NULL to avoid that close interface does > >>> not free some resource. > >>> > >>> Fixes: a135bc1644d6 ("net/nfp: fix resource leak for flower > >>> firmware") > >>> Cc: chaoyong.he@corigine.com > >>> Cc: stable@dpdk.org > >>> > >>> Signed-off-by: Long Wu <long.wu@corigine.com> > >>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com> > >>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com> > >>> --- > >>> drivers/net/nfp/flower/nfp_flower_representor.c | 15 > >>> ++++++++++++++- > >>> 1 file changed, 14 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c > >>> b/drivers/net/nfp/flower/nfp_flower_representor.c > >>> index 27ea3891bd..5f7c1fa737 100644 > >>> --- a/drivers/net/nfp/flower/nfp_flower_representor.c > >>> +++ b/drivers/net/nfp/flower/nfp_flower_representor.c > >>> @@ -294,17 +294,30 @@ nfp_flower_repr_tx_burst(void *tx_queue, > >>> static int nfp_flower_repr_uninit(struct rte_eth_dev *eth_dev) { > >>> + uint16_t index; > >>> struct nfp_flower_representor *repr; > >>> > >>> repr = eth_dev->data->dev_private; > >>> rte_ring_free(repr->ring); > >>> > >>> + if (repr->repr_type == NFP_REPR_TYPE_PHYS_PORT) { > >>> + index = NFP_FLOWER_CMSG_PORT_PHYS_PORT_NUM(repr- > >>> port_id); > >>> + repr->app_fw_flower->phy_reprs[index] = NULL; > >>> + } else { > >>> + index = repr->vf_id; > >>> + repr->app_fw_flower->vf_reprs[index] = NULL; > >>> + } > >>> + > >>> return 0; > >>> } > >>> > >>> static int > >>> -nfp_flower_pf_repr_uninit(__rte_unused struct rte_eth_dev *eth_dev) > >>> +nfp_flower_pf_repr_uninit(struct rte_eth_dev *eth_dev) > >>> { > >>> + struct nfp_flower_representor *repr = eth_dev->data->dev_private; > >>> + > >>> + repr->app_fw_flower->pf_repr = NULL; > >>> > >> > >> Here it is assigned to NULL but is it freed? If freed, why not set to > >> NULL where it is freed? > >> > >> Same for above phy_reprs & vf_reprs. > > > > The whole invoke view: > > rte_eth_dev_close() > > --> nfp_flower_repr_dev_close() > > --> nfp_flower_repr_free() > > --> nfp_flower_pf_repr_uninit() > > --> nfp_flower_repr_uninit() > > // In these two functions, we just assigned to NULL but not freed yet. > > // It is still refer by the `eth_dev->data->dev_private`. > > --> rte_eth_dev_release_port() > > --> rte_free(eth_dev->data->dev_private); > > // And here it is really freed (by the rte framework). > > > > 'rte_eth_dev_release_port()' frees the device private data, but not all pointers, > like 'repr->app_fw_flower->pf_repr', in the struct are freed, it is dev_close() or > unint() functions responsibility. > > Can you please double check if > 'eth_dev->data->dev_private->app_fw_flower->pf_repr' freed or not? (gdb) b nfp_flower_repr_dev_close Breakpoint 1 at 0x7f839a4ad37f: file ../drivers/net/nfp/flower/nfp_flower_representor.c, line 356. (gdb) c Continuing. Thread 1 "dpdk-testpmd" hit Breakpoint 1, nfp_flower_repr_dev_close (dev=0x7f839aed2340 <rte_eth_devices>) at ../drivers/net/nfp/flower/nfp_flower_representor.c:356 356 if (rte_eal_process_type() != RTE_PROC_PRIMARY) (gdb) n 359 repr = dev->data->dev_private; (gdb) 360 app_fw_flower = repr->app_fw_flower; (gdb) 361 hw = app_fw_flower->pf_hw; (gdb) 362 pf_dev = hw->pf_dev; (gdb) 368 nfp_net_disable_queues(dev); (gdb) p repr $1 = (struct nfp_flower_representor *) 0x17c49c800 (gdb) p dev->data->dev_private $2 = (void *) 0x17c49c800 (gdb) p repr->app_fw_flower->pf_repr $3 = (struct nfp_flower_representor *) 0x17c49c800 As we can see, these three pointers point the same block of memory. (gdb) until 384 nfp_flower_repr_dev_close (dev=0x7f839aed2340 <rte_eth_devices>) at ../drivers/net/nfp/flower/nfp_flower_representor.c:384 384 nfp_flower_repr_free(repr, repr->repr_type); (gdb) s nfp_flower_repr_free (repr=0x17c49c800, repr_type=NFP_REPR_TYPE_PF) at ../drivers/net/nfp/flower/nfp_flower_representor.c:328 328 switch (repr_type) { (gdb) n 333 nfp_flower_pf_repr_uninit(repr->eth_dev); (gdb) s nfp_flower_pf_repr_uninit (eth_dev=0x7f839aed2340 <rte_eth_devices>) at ../drivers/net/nfp/flower/nfp_flower_representor.c:317 317 struct nfp_flower_representor *repr = eth_dev->data->dev_private; (gdb) n 319 repr->app_fw_flower->pf_repr = NULL; (gdb) p eth_dev->data->dev_private $4 = (void *) 0x17c49c800 (gdb) p repr $5 = (struct nfp_flower_representor *) 0x17c49c800 (gdb) p repr->app_fw_flower->pf_repr $6 = (struct nfp_flower_representor *) 0x17c49c800 As what I said, although I assign NULL to one of the pointers `repr->app_fw_flower->pf_repr`, it still can be access by the other one `eth_dev->data->dev_private`. (gdb) fin Run till exit from #0 nfp_flower_pf_repr_uninit (eth_dev=0x7f839aed2340 <rte_eth_devices>) at ../drivers/net/nfp/flower/nfp_flower_representor.c:319 nfp_flower_repr_free (repr=0x17c49c800, repr_type=NFP_REPR_TYPE_PF) at ../drivers/net/nfp/flower/nfp_flower_representor.c:334 334 break; Value returned is $7 = 0 (gdb) fin Run till exit from #0 nfp_flower_repr_free (repr=0x17c49c800, repr_type=NFP_REPR_TYPE_PF) at ../drivers/net/nfp/flower/nfp_flower_representor.c:334 nfp_flower_repr_dev_close (dev=0x7f839aed2340 <rte_eth_devices>) at ../drivers/net/nfp/flower/nfp_flower_representor.c:386 386 for (i = 0; i < MAX_FLOWER_VFS; i++) { (gdb) b rte_eth_dev_release_port Breakpoint 2 at 0x7f839ae0d0d5: file ../lib/ethdev/ethdev_driver.c, line 230. (gdb) c Continuing. Thread 1 "dpdk-testpmd" hit Breakpoint 2, rte_eth_dev_release_port (eth_dev=0x7f839aed2340 <rte_eth_devices>) at ../lib/ethdev/ethdev_driver.c:230 230 if (eth_dev == NULL) (gdb) p eth_dev $8 = (struct rte_eth_dev *) 0x7f839aed2340 <rte_eth_devices> (gdb) until 267 rte_eth_dev_release_port (eth_dev=0x7f839aed2340 <rte_eth_devices>) at ../lib/ethdev/ethdev_driver.c:267 267 rte_free(eth_dev->data->dev_private); (gdb) p eth_dev->data->dev_private $9 = (void *) 0x17c49c800 (gdb) And here, we free this block of memory, and no memory leak happens, I think. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] net/nfp: fix free resource problem 2024-01-09 7:56 ` Chaoyong He @ 2024-01-09 17:48 ` Ferruh Yigit 2024-01-11 2:02 ` Chaoyong He 0 siblings, 1 reply; 14+ messages in thread From: Ferruh Yigit @ 2024-01-09 17:48 UTC (permalink / raw) To: Chaoyong He, dev; +Cc: oss-drivers, Long Wu, stable, Nole Zhang On 1/9/2024 7:56 AM, Chaoyong He wrote: >> On 12/18/2023 1:50 AM, Chaoyong He wrote: >>>> On 12/14/2023 10:24 AM, Chaoyong He wrote: >>>>> From: Long Wu <long.wu@corigine.com> >>>>> >>>>> Set the representor array to NULL to avoid that close interface does >>>>> not free some resource. >>>>> >>>>> Fixes: a135bc1644d6 ("net/nfp: fix resource leak for flower >>>>> firmware") >>>>> Cc: chaoyong.he@corigine.com >>>>> Cc: stable@dpdk.org >>>>> >>>>> Signed-off-by: Long Wu <long.wu@corigine.com> >>>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com> >>>>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com> >>>>> --- >>>>> drivers/net/nfp/flower/nfp_flower_representor.c | 15 >>>>> ++++++++++++++- >>>>> 1 file changed, 14 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c >>>>> b/drivers/net/nfp/flower/nfp_flower_representor.c >>>>> index 27ea3891bd..5f7c1fa737 100644 >>>>> --- a/drivers/net/nfp/flower/nfp_flower_representor.c >>>>> +++ b/drivers/net/nfp/flower/nfp_flower_representor.c >>>>> @@ -294,17 +294,30 @@ nfp_flower_repr_tx_burst(void *tx_queue, >>>>> static int nfp_flower_repr_uninit(struct rte_eth_dev *eth_dev) { >>>>> + uint16_t index; >>>>> struct nfp_flower_representor *repr; >>>>> >>>>> repr = eth_dev->data->dev_private; >>>>> rte_ring_free(repr->ring); >>>>> >>>>> + if (repr->repr_type == NFP_REPR_TYPE_PHYS_PORT) { >>>>> + index = NFP_FLOWER_CMSG_PORT_PHYS_PORT_NUM(repr- >>>>> port_id); >>>>> + repr->app_fw_flower->phy_reprs[index] = NULL; >>>>> + } else { >>>>> + index = repr->vf_id; >>>>> + repr->app_fw_flower->vf_reprs[index] = NULL; >>>>> + } >>>>> + >>>>> return 0; >>>>> } >>>>> >>>>> static int >>>>> -nfp_flower_pf_repr_uninit(__rte_unused struct rte_eth_dev *eth_dev) >>>>> +nfp_flower_pf_repr_uninit(struct rte_eth_dev *eth_dev) >>>>> { >>>>> + struct nfp_flower_representor *repr = eth_dev->data->dev_private; >>>>> + >>>>> + repr->app_fw_flower->pf_repr = NULL; >>>>> >>>> >>>> Here it is assigned to NULL but is it freed? If freed, why not set to >>>> NULL where it is freed? >>>> >>>> Same for above phy_reprs & vf_reprs. >>> >>> The whole invoke view: >>> rte_eth_dev_close() >>> --> nfp_flower_repr_dev_close() >>> --> nfp_flower_repr_free() >>> --> nfp_flower_pf_repr_uninit() >>> --> nfp_flower_repr_uninit() >>> // In these two functions, we just assigned to NULL but not freed yet. >>> // It is still refer by the `eth_dev->data->dev_private`. >>> --> rte_eth_dev_release_port() >>> --> rte_free(eth_dev->data->dev_private); >>> // And here it is really freed (by the rte framework). >>> >> >> 'rte_eth_dev_release_port()' frees the device private data, but not all pointers, >> like 'repr->app_fw_flower->pf_repr', in the struct are freed, it is dev_close() or >> unint() functions responsibility. >> >> Can you please double check if >> 'eth_dev->data->dev_private->app_fw_flower->pf_repr' freed or not? > > (gdb) b nfp_flower_repr_dev_close > Breakpoint 1 at 0x7f839a4ad37f: file ../drivers/net/nfp/flower/nfp_flower_representor.c, line 356. > (gdb) c > Continuing. > > Thread 1 "dpdk-testpmd" hit Breakpoint 1, nfp_flower_repr_dev_close (dev=0x7f839aed2340 <rte_eth_devices>) > at ../drivers/net/nfp/flower/nfp_flower_representor.c:356 > 356 if (rte_eal_process_type() != RTE_PROC_PRIMARY) > (gdb) n > 359 repr = dev->data->dev_private; > (gdb) > 360 app_fw_flower = repr->app_fw_flower; > (gdb) > 361 hw = app_fw_flower->pf_hw; > (gdb) > 362 pf_dev = hw->pf_dev; > (gdb) > 368 nfp_net_disable_queues(dev); > (gdb) p repr > $1 = (struct nfp_flower_representor *) 0x17c49c800 > (gdb) p dev->data->dev_private > $2 = (void *) 0x17c49c800 > (gdb) p repr->app_fw_flower->pf_repr > $3 = (struct nfp_flower_representor *) 0x17c49c800 > > As we can see, these three pointers point the same block of memory. > Ahh, I missed that 'repr->app_fw_flower->pf_repr' points to 'dev_private', so your code makes sense. But if it is 'dev_private', why free it in 'nfp_pf_uninit()' as it will be freed by 'rte_eth_dev_release_port()'? Won't removing 'rte_free(pf_dev);' from 'nfp_pf_uninit()' will have the same effect, instead of setting it NULL in advance? ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 2/3] net/nfp: fix free resource problem 2024-01-09 17:48 ` Ferruh Yigit @ 2024-01-11 2:02 ` Chaoyong He 2024-01-11 12:32 ` Ferruh Yigit 0 siblings, 1 reply; 14+ messages in thread From: Chaoyong He @ 2024-01-11 2:02 UTC (permalink / raw) To: Ferruh Yigit, dev; +Cc: oss-drivers, Long Wu, stable, Nole Zhang > On 1/9/2024 7:56 AM, Chaoyong He wrote: > >> On 12/18/2023 1:50 AM, Chaoyong He wrote: > >>>> On 12/14/2023 10:24 AM, Chaoyong He wrote: > >>>>> From: Long Wu <long.wu@corigine.com> > >>>>> > >>>>> Set the representor array to NULL to avoid that close interface > >>>>> does not free some resource. > >>>>> > >>>>> Fixes: a135bc1644d6 ("net/nfp: fix resource leak for flower > >>>>> firmware") > >>>>> Cc: chaoyong.he@corigine.com > >>>>> Cc: stable@dpdk.org > >>>>> > >>>>> Signed-off-by: Long Wu <long.wu@corigine.com> > >>>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com> > >>>>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com> > >>>>> --- > >>>>> drivers/net/nfp/flower/nfp_flower_representor.c | 15 > >>>>> ++++++++++++++- > >>>>> 1 file changed, 14 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c > >>>>> b/drivers/net/nfp/flower/nfp_flower_representor.c > >>>>> index 27ea3891bd..5f7c1fa737 100644 > >>>>> --- a/drivers/net/nfp/flower/nfp_flower_representor.c > >>>>> +++ b/drivers/net/nfp/flower/nfp_flower_representor.c > >>>>> @@ -294,17 +294,30 @@ nfp_flower_repr_tx_burst(void *tx_queue, > >>>>> static int nfp_flower_repr_uninit(struct rte_eth_dev *eth_dev) { > >>>>> + uint16_t index; > >>>>> struct nfp_flower_representor *repr; > >>>>> > >>>>> repr = eth_dev->data->dev_private; > >>>>> rte_ring_free(repr->ring); > >>>>> > >>>>> + if (repr->repr_type == NFP_REPR_TYPE_PHYS_PORT) { > >>>>> + index = NFP_FLOWER_CMSG_PORT_PHYS_PORT_NUM(repr- > >>>>> port_id); > >>>>> + repr->app_fw_flower->phy_reprs[index] = NULL; > >>>>> + } else { > >>>>> + index = repr->vf_id; > >>>>> + repr->app_fw_flower->vf_reprs[index] = NULL; > >>>>> + } > >>>>> + > >>>>> return 0; > >>>>> } > >>>>> > >>>>> static int > >>>>> -nfp_flower_pf_repr_uninit(__rte_unused struct rte_eth_dev > >>>>> *eth_dev) > >>>>> +nfp_flower_pf_repr_uninit(struct rte_eth_dev *eth_dev) > >>>>> { > >>>>> + struct nfp_flower_representor *repr = > >>>>> +eth_dev->data->dev_private; > >>>>> + > >>>>> + repr->app_fw_flower->pf_repr = NULL; > >>>>> > >>>> > >>>> Here it is assigned to NULL but is it freed? If freed, why not set > >>>> to NULL where it is freed? > >>>> > >>>> Same for above phy_reprs & vf_reprs. > >>> > >>> The whole invoke view: > >>> rte_eth_dev_close() > >>> --> nfp_flower_repr_dev_close() > >>> --> nfp_flower_repr_free() > >>> --> nfp_flower_pf_repr_uninit() > >>> --> nfp_flower_repr_uninit() > >>> // In these two functions, we just assigned to NULL but not freed > yet. > >>> // It is still refer by the `eth_dev->data->dev_private`. > >>> --> rte_eth_dev_release_port() > >>> --> rte_free(eth_dev->data->dev_private); > >>> // And here it is really freed (by the rte framework). > >>> > >> > >> 'rte_eth_dev_release_port()' frees the device private data, but not > >> all pointers, like 'repr->app_fw_flower->pf_repr', in the struct are > >> freed, it is dev_close() or > >> unint() functions responsibility. > >> > >> Can you please double check if > >> 'eth_dev->data->dev_private->app_fw_flower->pf_repr' freed or not? > > > > (gdb) b nfp_flower_repr_dev_close > > Breakpoint 1 at 0x7f839a4ad37f: > file ../drivers/net/nfp/flower/nfp_flower_representor.c, line 356. > > (gdb) c > > Continuing. > > > > Thread 1 "dpdk-testpmd" hit Breakpoint 1, nfp_flower_repr_dev_close > (dev=0x7f839aed2340 <rte_eth_devices>) > > at ../drivers/net/nfp/flower/nfp_flower_representor.c:356 > > 356 if (rte_eal_process_type() != RTE_PROC_PRIMARY) > > (gdb) n > > 359 repr = dev->data->dev_private; > > (gdb) > > 360 app_fw_flower = repr->app_fw_flower; > > (gdb) > > 361 hw = app_fw_flower->pf_hw; > > (gdb) > > 362 pf_dev = hw->pf_dev; > > (gdb) > > 368 nfp_net_disable_queues(dev); > > (gdb) p repr > > $1 = (struct nfp_flower_representor *) 0x17c49c800 > > (gdb) p dev->data->dev_private > > $2 = (void *) 0x17c49c800 > > (gdb) p repr->app_fw_flower->pf_repr > > $3 = (struct nfp_flower_representor *) 0x17c49c800 > > > > As we can see, these three pointers point the same block of memory. > > > > Ahh, I missed that 'repr->app_fw_flower->pf_repr' points to 'dev_private', so > your code makes sense. > > But if it is 'dev_private', why free it in 'nfp_pf_uninit()' as it will be freed by > 'rte_eth_dev_release_port()'? Sorry, I'm not understanding this. The 'dev_private' is a 'struct nfp_flower_representor *', and it will be freed in 'rte_eth_dev_release_port()'. What I freed in 'nfp_pf_uninit()' is a 'struct nfp_pf_dev *', so I'm not catch your point about this. > Won't removing 'rte_free(pf_dev);' from 'nfp_pf_uninit()' will have the same > effect, instead of setting it NULL in advance? > If I remove the 'rte_free(pf_dev);' from 'nfp_pf_uninit()', there will be a memory leak as no one will free it, and actually I'm not 'setting it NULL in advance'. 359 repr = dev->data->dev_private; 360 app_fw_flower = repr->app_fw_flower; 361 hw = app_fw_flower->pf_hw; 362 pf_dev = hw->pf_dev; Maybe you just confuse the 'pf_repr' and 'pf_dev'? Just a guess. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] net/nfp: fix free resource problem 2024-01-11 2:02 ` Chaoyong He @ 2024-01-11 12:32 ` Ferruh Yigit 2024-01-12 1:19 ` Chaoyong He 0 siblings, 1 reply; 14+ messages in thread From: Ferruh Yigit @ 2024-01-11 12:32 UTC (permalink / raw) To: Chaoyong He, dev; +Cc: oss-drivers, Long Wu, stable, Nole Zhang On 1/11/2024 2:02 AM, Chaoyong He wrote: >> On 1/9/2024 7:56 AM, Chaoyong He wrote: >>>> On 12/18/2023 1:50 AM, Chaoyong He wrote: >>>>>> On 12/14/2023 10:24 AM, Chaoyong He wrote: >>>>>>> From: Long Wu <long.wu@corigine.com> >>>>>>> >>>>>>> Set the representor array to NULL to avoid that close interface >>>>>>> does not free some resource. >>>>>>> >>>>>>> Fixes: a135bc1644d6 ("net/nfp: fix resource leak for flower >>>>>>> firmware") >>>>>>> Cc: chaoyong.he@corigine.com >>>>>>> Cc: stable@dpdk.org >>>>>>> >>>>>>> Signed-off-by: Long Wu <long.wu@corigine.com> >>>>>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com> >>>>>>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com> >>>>>>> --- >>>>>>> drivers/net/nfp/flower/nfp_flower_representor.c | 15 >>>>>>> ++++++++++++++- >>>>>>> 1 file changed, 14 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c >>>>>>> b/drivers/net/nfp/flower/nfp_flower_representor.c >>>>>>> index 27ea3891bd..5f7c1fa737 100644 >>>>>>> --- a/drivers/net/nfp/flower/nfp_flower_representor.c >>>>>>> +++ b/drivers/net/nfp/flower/nfp_flower_representor.c >>>>>>> @@ -294,17 +294,30 @@ nfp_flower_repr_tx_burst(void *tx_queue, >>>>>>> static int nfp_flower_repr_uninit(struct rte_eth_dev *eth_dev) { >>>>>>> + uint16_t index; >>>>>>> struct nfp_flower_representor *repr; >>>>>>> >>>>>>> repr = eth_dev->data->dev_private; >>>>>>> rte_ring_free(repr->ring); >>>>>>> >>>>>>> + if (repr->repr_type == NFP_REPR_TYPE_PHYS_PORT) { >>>>>>> + index = NFP_FLOWER_CMSG_PORT_PHYS_PORT_NUM(repr- >>>>>>> port_id); >>>>>>> + repr->app_fw_flower->phy_reprs[index] = NULL; >>>>>>> + } else { >>>>>>> + index = repr->vf_id; >>>>>>> + repr->app_fw_flower->vf_reprs[index] = NULL; >>>>>>> + } >>>>>>> + >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> static int >>>>>>> -nfp_flower_pf_repr_uninit(__rte_unused struct rte_eth_dev >>>>>>> *eth_dev) >>>>>>> +nfp_flower_pf_repr_uninit(struct rte_eth_dev *eth_dev) >>>>>>> { >>>>>>> + struct nfp_flower_representor *repr = >>>>>>> +eth_dev->data->dev_private; >>>>>>> + >>>>>>> + repr->app_fw_flower->pf_repr = NULL; >>>>>>> >>>>>> >>>>>> Here it is assigned to NULL but is it freed? If freed, why not set >>>>>> to NULL where it is freed? >>>>>> >>>>>> Same for above phy_reprs & vf_reprs. >>>>> >>>>> The whole invoke view: >>>>> rte_eth_dev_close() >>>>> --> nfp_flower_repr_dev_close() >>>>> --> nfp_flower_repr_free() >>>>> --> nfp_flower_pf_repr_uninit() >>>>> --> nfp_flower_repr_uninit() >>>>> // In these two functions, we just assigned to NULL but not freed >> yet. >>>>> // It is still refer by the `eth_dev->data->dev_private`. >>>>> --> rte_eth_dev_release_port() >>>>> --> rte_free(eth_dev->data->dev_private); >>>>> // And here it is really freed (by the rte framework). >>>>> >>>> >>>> 'rte_eth_dev_release_port()' frees the device private data, but not >>>> all pointers, like 'repr->app_fw_flower->pf_repr', in the struct are >>>> freed, it is dev_close() or >>>> unint() functions responsibility. >>>> >>>> Can you please double check if >>>> 'eth_dev->data->dev_private->app_fw_flower->pf_repr' freed or not? >>> >>> (gdb) b nfp_flower_repr_dev_close >>> Breakpoint 1 at 0x7f839a4ad37f: >> file ../drivers/net/nfp/flower/nfp_flower_representor.c, line 356. >>> (gdb) c >>> Continuing. >>> >>> Thread 1 "dpdk-testpmd" hit Breakpoint 1, nfp_flower_repr_dev_close >> (dev=0x7f839aed2340 <rte_eth_devices>) >>> at ../drivers/net/nfp/flower/nfp_flower_representor.c:356 >>> 356 if (rte_eal_process_type() != RTE_PROC_PRIMARY) >>> (gdb) n >>> 359 repr = dev->data->dev_private; >>> (gdb) >>> 360 app_fw_flower = repr->app_fw_flower; >>> (gdb) >>> 361 hw = app_fw_flower->pf_hw; >>> (gdb) >>> 362 pf_dev = hw->pf_dev; >>> (gdb) >>> 368 nfp_net_disable_queues(dev); >>> (gdb) p repr >>> $1 = (struct nfp_flower_representor *) 0x17c49c800 >>> (gdb) p dev->data->dev_private >>> $2 = (void *) 0x17c49c800 >>> (gdb) p repr->app_fw_flower->pf_repr >>> $3 = (struct nfp_flower_representor *) 0x17c49c800 >>> >>> As we can see, these three pointers point the same block of memory. >>> >> >> Ahh, I missed that 'repr->app_fw_flower->pf_repr' points to 'dev_private', so >> your code makes sense. >> >> But if it is 'dev_private', why free it in 'nfp_pf_uninit()' as it will be freed by >> 'rte_eth_dev_release_port()'? > > Sorry, I'm not understanding this. > The 'dev_private' is a 'struct nfp_flower_representor *', and it will be freed in 'rte_eth_dev_release_port()'. > What I freed in 'nfp_pf_uninit()' is a 'struct nfp_pf_dev *', so I'm not catch your point about this. > >> Won't removing 'rte_free(pf_dev);' from 'nfp_pf_uninit()' will have the same >> effect, instead of setting it NULL in advance? >> > > If I remove the 'rte_free(pf_dev);' from 'nfp_pf_uninit()', there will be a memory leak as no one will free it, and actually I'm not 'setting it NULL in advance'. > > 359 repr = dev->data->dev_private; > 360 app_fw_flower = repr->app_fw_flower; > 361 hw = app_fw_flower->pf_hw; > 362 pf_dev = hw->pf_dev; > > Maybe you just confuse the 'pf_repr' and 'pf_dev'? Just a guess. > Yes I did confuse those two, sorry about that. 'repr->app_fw_flower->pf_repr' is 'dev_private', and I assumed you are setting it NULL to escape from double free (and was checking where that double free happens), but I guess that is not the case. 'rte_eth_dev_destroy()' calls 'rte_eth_dev_release_port()' and frees 'dev_private' but 'repr->app_fw_flower->pf_repr' remains as dangling pointer and perhaps prevents 'nfp_flower_repr_dev_close()' move forward (because of "if (app_fw_flower->pf_repr != NULL)" check), and you are fixing it, is it the case? ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 2/3] net/nfp: fix free resource problem 2024-01-11 12:32 ` Ferruh Yigit @ 2024-01-12 1:19 ` Chaoyong He 2024-01-12 10:06 ` Ferruh Yigit 0 siblings, 1 reply; 14+ messages in thread From: Chaoyong He @ 2024-01-12 1:19 UTC (permalink / raw) To: Ferruh Yigit, dev; +Cc: oss-drivers, Long Wu, stable, Nole Zhang > On 1/11/2024 2:02 AM, Chaoyong He wrote: > >> On 1/9/2024 7:56 AM, Chaoyong He wrote: > >>>> On 12/18/2023 1:50 AM, Chaoyong He wrote: > >>>>>> On 12/14/2023 10:24 AM, Chaoyong He wrote: > >>>>>>> From: Long Wu <long.wu@corigine.com> > >>>>>>> > >>>>>>> Set the representor array to NULL to avoid that close interface > >>>>>>> does not free some resource. > >>>>>>> > >>>>>>> Fixes: a135bc1644d6 ("net/nfp: fix resource leak for flower > >>>>>>> firmware") > >>>>>>> Cc: chaoyong.he@corigine.com > >>>>>>> Cc: stable@dpdk.org > >>>>>>> > >>>>>>> Signed-off-by: Long Wu <long.wu@corigine.com> > >>>>>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com> > >>>>>>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com> > >>>>>>> --- > >>>>>>> drivers/net/nfp/flower/nfp_flower_representor.c | 15 > >>>>>>> ++++++++++++++- > >>>>>>> 1 file changed, 14 insertions(+), 1 deletion(-) > >>>>>>> > >>>>>>> diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c > >>>>>>> b/drivers/net/nfp/flower/nfp_flower_representor.c > >>>>>>> index 27ea3891bd..5f7c1fa737 100644 > >>>>>>> --- a/drivers/net/nfp/flower/nfp_flower_representor.c > >>>>>>> +++ b/drivers/net/nfp/flower/nfp_flower_representor.c > >>>>>>> @@ -294,17 +294,30 @@ nfp_flower_repr_tx_burst(void > *tx_queue, > >>>>>>> static int nfp_flower_repr_uninit(struct rte_eth_dev *eth_dev) > >>>>>>> { > >>>>>>> + uint16_t index; > >>>>>>> struct nfp_flower_representor *repr; > >>>>>>> > >>>>>>> repr = eth_dev->data->dev_private; > >>>>>>> rte_ring_free(repr->ring); > >>>>>>> > >>>>>>> + if (repr->repr_type == NFP_REPR_TYPE_PHYS_PORT) { > >>>>>>> + index = > NFP_FLOWER_CMSG_PORT_PHYS_PORT_NUM(repr- > >>>>>>> port_id); > >>>>>>> + repr->app_fw_flower->phy_reprs[index] = NULL; > >>>>>>> + } else { > >>>>>>> + index = repr->vf_id; > >>>>>>> + repr->app_fw_flower->vf_reprs[index] = NULL; > >>>>>>> + } > >>>>>>> + > >>>>>>> return 0; > >>>>>>> } > >>>>>>> > >>>>>>> static int > >>>>>>> -nfp_flower_pf_repr_uninit(__rte_unused struct rte_eth_dev > >>>>>>> *eth_dev) > >>>>>>> +nfp_flower_pf_repr_uninit(struct rte_eth_dev *eth_dev) > >>>>>>> { > >>>>>>> + struct nfp_flower_representor *repr = > >>>>>>> +eth_dev->data->dev_private; > >>>>>>> + > >>>>>>> + repr->app_fw_flower->pf_repr = NULL; > >>>>>>> > >>>>>> > >>>>>> Here it is assigned to NULL but is it freed? If freed, why not > >>>>>> set to NULL where it is freed? > >>>>>> > >>>>>> Same for above phy_reprs & vf_reprs. > >>>>> > >>>>> The whole invoke view: > >>>>> rte_eth_dev_close() > >>>>> --> nfp_flower_repr_dev_close() > >>>>> --> nfp_flower_repr_free() > >>>>> --> nfp_flower_pf_repr_uninit() > >>>>> --> nfp_flower_repr_uninit() > >>>>> // In these two functions, we just assigned to NULL but > >>>>> not freed > >> yet. > >>>>> // It is still refer by the `eth_dev->data->dev_private`. > >>>>> --> rte_eth_dev_release_port() > >>>>> --> rte_free(eth_dev->data->dev_private); > >>>>> // And here it is really freed (by the rte framework). > >>>>> > >>>> > >>>> 'rte_eth_dev_release_port()' frees the device private data, but not > >>>> all pointers, like 'repr->app_fw_flower->pf_repr', in the struct > >>>> are freed, it is dev_close() or > >>>> unint() functions responsibility. > >>>> > >>>> Can you please double check if > >>>> 'eth_dev->data->dev_private->app_fw_flower->pf_repr' freed or not? > >>> > >>> (gdb) b nfp_flower_repr_dev_close > >>> Breakpoint 1 at 0x7f839a4ad37f: > >> file ../drivers/net/nfp/flower/nfp_flower_representor.c, line 356. > >>> (gdb) c > >>> Continuing. > >>> > >>> Thread 1 "dpdk-testpmd" hit Breakpoint 1, nfp_flower_repr_dev_close > >> (dev=0x7f839aed2340 <rte_eth_devices>) > >>> at ../drivers/net/nfp/flower/nfp_flower_representor.c:356 > >>> 356 if (rte_eal_process_type() != RTE_PROC_PRIMARY) > >>> (gdb) n > >>> 359 repr = dev->data->dev_private; > >>> (gdb) > >>> 360 app_fw_flower = repr->app_fw_flower; > >>> (gdb) > >>> 361 hw = app_fw_flower->pf_hw; > >>> (gdb) > >>> 362 pf_dev = hw->pf_dev; > >>> (gdb) > >>> 368 nfp_net_disable_queues(dev); > >>> (gdb) p repr > >>> $1 = (struct nfp_flower_representor *) 0x17c49c800 > >>> (gdb) p dev->data->dev_private > >>> $2 = (void *) 0x17c49c800 > >>> (gdb) p repr->app_fw_flower->pf_repr > >>> $3 = (struct nfp_flower_representor *) 0x17c49c800 > >>> > >>> As we can see, these three pointers point the same block of memory. > >>> > >> > >> Ahh, I missed that 'repr->app_fw_flower->pf_repr' points to > >> 'dev_private', so your code makes sense. > >> > >> But if it is 'dev_private', why free it in 'nfp_pf_uninit()' as it > >> will be freed by 'rte_eth_dev_release_port()'? > > > > Sorry, I'm not understanding this. > > The 'dev_private' is a 'struct nfp_flower_representor *', and it will be freed in > 'rte_eth_dev_release_port()'. > > What I freed in 'nfp_pf_uninit()' is a 'struct nfp_pf_dev *', so I'm not catch > your point about this. > > > >> Won't removing 'rte_free(pf_dev);' from 'nfp_pf_uninit()' will have > >> the same effect, instead of setting it NULL in advance? > >> > > > > If I remove the 'rte_free(pf_dev);' from 'nfp_pf_uninit()', there will be a > memory leak as no one will free it, and actually I'm not 'setting it NULL in > advance'. > > > > 359 repr = dev->data->dev_private; > > 360 app_fw_flower = repr->app_fw_flower; > > 361 hw = app_fw_flower->pf_hw; > > 362 pf_dev = hw->pf_dev; > > > > Maybe you just confuse the 'pf_repr' and 'pf_dev'? Just a guess. > > > > Yes I did confuse those two, sorry about that. > > 'repr->app_fw_flower->pf_repr' is 'dev_private', and I assumed you are setting > it NULL to escape from double free (and was checking where that double free > happens), but I guess that is not the case. > > 'rte_eth_dev_destroy()' calls 'rte_eth_dev_release_port()' and frees > 'dev_private' but 'repr->app_fw_flower->pf_repr' remains as dangling pointer > and perhaps prevents 'nfp_flower_repr_dev_close()' move forward (because > of "if (app_fw_flower->pf_repr != NULL)" check), and you are fixing it, is it the > case? Correct, that's what we want to do by this patch and where the problem is, your description is very clear and brief. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] net/nfp: fix free resource problem 2024-01-12 1:19 ` Chaoyong He @ 2024-01-12 10:06 ` Ferruh Yigit 0 siblings, 0 replies; 14+ messages in thread From: Ferruh Yigit @ 2024-01-12 10:06 UTC (permalink / raw) To: Chaoyong He, dev; +Cc: oss-drivers, Long Wu, stable, Nole Zhang On 1/12/2024 1:19 AM, Chaoyong He wrote: >> On 1/11/2024 2:02 AM, Chaoyong He wrote: >>>> On 1/9/2024 7:56 AM, Chaoyong He wrote: >>>>>> On 12/18/2023 1:50 AM, Chaoyong He wrote: >>>>>>>> On 12/14/2023 10:24 AM, Chaoyong He wrote: >>>>>>>>> From: Long Wu <long.wu@corigine.com> >>>>>>>>> >>>>>>>>> Set the representor array to NULL to avoid that close interface >>>>>>>>> does not free some resource. >>>>>>>>> >>>>>>>>> Fixes: a135bc1644d6 ("net/nfp: fix resource leak for flower >>>>>>>>> firmware") >>>>>>>>> Cc: chaoyong.he@corigine.com >>>>>>>>> Cc: stable@dpdk.org >>>>>>>>> >>>>>>>>> Signed-off-by: Long Wu <long.wu@corigine.com> >>>>>>>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com> >>>>>>>>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com> >>>>>>>>> --- >>>>>>>>> drivers/net/nfp/flower/nfp_flower_representor.c | 15 >>>>>>>>> ++++++++++++++- >>>>>>>>> 1 file changed, 14 insertions(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c >>>>>>>>> b/drivers/net/nfp/flower/nfp_flower_representor.c >>>>>>>>> index 27ea3891bd..5f7c1fa737 100644 >>>>>>>>> --- a/drivers/net/nfp/flower/nfp_flower_representor.c >>>>>>>>> +++ b/drivers/net/nfp/flower/nfp_flower_representor.c >>>>>>>>> @@ -294,17 +294,30 @@ nfp_flower_repr_tx_burst(void >> *tx_queue, >>>>>>>>> static int nfp_flower_repr_uninit(struct rte_eth_dev *eth_dev) >>>>>>>>> { >>>>>>>>> + uint16_t index; >>>>>>>>> struct nfp_flower_representor *repr; >>>>>>>>> >>>>>>>>> repr = eth_dev->data->dev_private; >>>>>>>>> rte_ring_free(repr->ring); >>>>>>>>> >>>>>>>>> + if (repr->repr_type == NFP_REPR_TYPE_PHYS_PORT) { >>>>>>>>> + index = >> NFP_FLOWER_CMSG_PORT_PHYS_PORT_NUM(repr- >>>>>>>>> port_id); >>>>>>>>> + repr->app_fw_flower->phy_reprs[index] = NULL; >>>>>>>>> + } else { >>>>>>>>> + index = repr->vf_id; >>>>>>>>> + repr->app_fw_flower->vf_reprs[index] = NULL; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> return 0; >>>>>>>>> } >>>>>>>>> >>>>>>>>> static int >>>>>>>>> -nfp_flower_pf_repr_uninit(__rte_unused struct rte_eth_dev >>>>>>>>> *eth_dev) >>>>>>>>> +nfp_flower_pf_repr_uninit(struct rte_eth_dev *eth_dev) >>>>>>>>> { >>>>>>>>> + struct nfp_flower_representor *repr = >>>>>>>>> +eth_dev->data->dev_private; >>>>>>>>> + >>>>>>>>> + repr->app_fw_flower->pf_repr = NULL; >>>>>>>>> >>>>>>>> >>>>>>>> Here it is assigned to NULL but is it freed? If freed, why not >>>>>>>> set to NULL where it is freed? >>>>>>>> >>>>>>>> Same for above phy_reprs & vf_reprs. >>>>>>> >>>>>>> The whole invoke view: >>>>>>> rte_eth_dev_close() >>>>>>> --> nfp_flower_repr_dev_close() >>>>>>> --> nfp_flower_repr_free() >>>>>>> --> nfp_flower_pf_repr_uninit() >>>>>>> --> nfp_flower_repr_uninit() >>>>>>> // In these two functions, we just assigned to NULL but >>>>>>> not freed >>>> yet. >>>>>>> // It is still refer by the `eth_dev->data->dev_private`. >>>>>>> --> rte_eth_dev_release_port() >>>>>>> --> rte_free(eth_dev->data->dev_private); >>>>>>> // And here it is really freed (by the rte framework). >>>>>>> >>>>>> >>>>>> 'rte_eth_dev_release_port()' frees the device private data, but not >>>>>> all pointers, like 'repr->app_fw_flower->pf_repr', in the struct >>>>>> are freed, it is dev_close() or >>>>>> unint() functions responsibility. >>>>>> >>>>>> Can you please double check if >>>>>> 'eth_dev->data->dev_private->app_fw_flower->pf_repr' freed or not? >>>>> >>>>> (gdb) b nfp_flower_repr_dev_close >>>>> Breakpoint 1 at 0x7f839a4ad37f: >>>> file ../drivers/net/nfp/flower/nfp_flower_representor.c, line 356. >>>>> (gdb) c >>>>> Continuing. >>>>> >>>>> Thread 1 "dpdk-testpmd" hit Breakpoint 1, nfp_flower_repr_dev_close >>>> (dev=0x7f839aed2340 <rte_eth_devices>) >>>>> at ../drivers/net/nfp/flower/nfp_flower_representor.c:356 >>>>> 356 if (rte_eal_process_type() != RTE_PROC_PRIMARY) >>>>> (gdb) n >>>>> 359 repr = dev->data->dev_private; >>>>> (gdb) >>>>> 360 app_fw_flower = repr->app_fw_flower; >>>>> (gdb) >>>>> 361 hw = app_fw_flower->pf_hw; >>>>> (gdb) >>>>> 362 pf_dev = hw->pf_dev; >>>>> (gdb) >>>>> 368 nfp_net_disable_queues(dev); >>>>> (gdb) p repr >>>>> $1 = (struct nfp_flower_representor *) 0x17c49c800 >>>>> (gdb) p dev->data->dev_private >>>>> $2 = (void *) 0x17c49c800 >>>>> (gdb) p repr->app_fw_flower->pf_repr >>>>> $3 = (struct nfp_flower_representor *) 0x17c49c800 >>>>> >>>>> As we can see, these three pointers point the same block of memory. >>>>> >>>> >>>> Ahh, I missed that 'repr->app_fw_flower->pf_repr' points to >>>> 'dev_private', so your code makes sense. >>>> >>>> But if it is 'dev_private', why free it in 'nfp_pf_uninit()' as it >>>> will be freed by 'rte_eth_dev_release_port()'? >>> >>> Sorry, I'm not understanding this. >>> The 'dev_private' is a 'struct nfp_flower_representor *', and it will be freed in >> 'rte_eth_dev_release_port()'. >>> What I freed in 'nfp_pf_uninit()' is a 'struct nfp_pf_dev *', so I'm not catch >> your point about this. >>> >>>> Won't removing 'rte_free(pf_dev);' from 'nfp_pf_uninit()' will have >>>> the same effect, instead of setting it NULL in advance? >>>> >>> >>> If I remove the 'rte_free(pf_dev);' from 'nfp_pf_uninit()', there will be a >> memory leak as no one will free it, and actually I'm not 'setting it NULL in >> advance'. >>> >>> 359 repr = dev->data->dev_private; >>> 360 app_fw_flower = repr->app_fw_flower; >>> 361 hw = app_fw_flower->pf_hw; >>> 362 pf_dev = hw->pf_dev; >>> >>> Maybe you just confuse the 'pf_repr' and 'pf_dev'? Just a guess. >>> >> >> Yes I did confuse those two, sorry about that. >> >> 'repr->app_fw_flower->pf_repr' is 'dev_private', and I assumed you are setting >> it NULL to escape from double free (and was checking where that double free >> happens), but I guess that is not the case. >> >> 'rte_eth_dev_destroy()' calls 'rte_eth_dev_release_port()' and frees >> 'dev_private' but 'repr->app_fw_flower->pf_repr' remains as dangling pointer >> and perhaps prevents 'nfp_flower_repr_dev_close()' move forward (because >> of "if (app_fw_flower->pf_repr != NULL)" check), and you are fixing it, is it the >> case? > > Correct, that's what we want to do by this patch and where the problem is, your description is very clear and brief. > Got it, I will proceed with the set. More details in the commit log helps reviewing the patches. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] net/nfp: free domain ID in close interface 2023-12-14 10:24 [PATCH 0/3] fix some problems of flower firmware Chaoyong He 2023-12-14 10:24 ` [PATCH 1/3] net/nfp: fix close representor problem Chaoyong He 2023-12-14 10:24 ` [PATCH 2/3] net/nfp: fix free resource problem Chaoyong He @ 2023-12-14 10:24 ` Chaoyong He 2024-01-12 12:05 ` [PATCH 0/3] fix some problems of flower firmware Ferruh Yigit 3 siblings, 0 replies; 14+ messages in thread From: Chaoyong He @ 2023-12-14 10:24 UTC (permalink / raw) To: dev; +Cc: oss-drivers, Long Wu, chaoyong.he, stable, Peng Zhang From: Long Wu <long.wu@corigine.com> Free domain id in close interface. Fixes: e1124c4f8a45 ("net/nfp: add flower representor framework") Cc: chaoyong.he@corigine.com Cc: stable@dpdk.org Signed-off-by: Long Wu <long.wu@corigine.com> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com> Reviewed-by: Peng Zhang <peng.zhang@corigine.com> --- drivers/net/nfp/flower/nfp_flower.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/nfp/flower/nfp_flower.c b/drivers/net/nfp/flower/nfp_flower.c index 94b50611f0..88a1b061e1 100644 --- a/drivers/net/nfp/flower/nfp_flower.c +++ b/drivers/net/nfp/flower/nfp_flower.c @@ -814,6 +814,7 @@ nfp_uninit_app_fw_flower(struct nfp_pf_dev *pf_dev) rte_free(app_fw_flower->pf_hw); nfp_mtr_priv_uninit(pf_dev); nfp_flow_priv_uninit(pf_dev); + rte_eth_switch_domain_free(app_fw_flower->switch_domain_id); rte_free(app_fw_flower); } -- 2.39.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] fix some problems of flower firmware 2023-12-14 10:24 [PATCH 0/3] fix some problems of flower firmware Chaoyong He ` (2 preceding siblings ...) 2023-12-14 10:24 ` [PATCH 3/3] net/nfp: free domain ID in close interface Chaoyong He @ 2024-01-12 12:05 ` Ferruh Yigit 3 siblings, 0 replies; 14+ messages in thread From: Ferruh Yigit @ 2024-01-12 12:05 UTC (permalink / raw) To: Chaoyong He, dev; +Cc: oss-drivers On 12/14/2023 10:24 AM, Chaoyong He wrote: > This patch series fix some problems of flower firmware. > > Long Wu (3): > net/nfp: fix close representor problem > net/nfp: fix free resource problem > net/nfp: free domain ID in close interface > Series applied to dpdk-next-net/main, thanks. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-01-12 12:06 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-12-14 10:24 [PATCH 0/3] fix some problems of flower firmware Chaoyong He 2023-12-14 10:24 ` [PATCH 1/3] net/nfp: fix close representor problem Chaoyong He 2023-12-14 10:24 ` [PATCH 2/3] net/nfp: fix free resource problem Chaoyong He 2023-12-15 12:54 ` Ferruh Yigit 2023-12-18 1:50 ` Chaoyong He 2024-01-08 15:42 ` Ferruh Yigit 2024-01-09 7:56 ` Chaoyong He 2024-01-09 17:48 ` Ferruh Yigit 2024-01-11 2:02 ` Chaoyong He 2024-01-11 12:32 ` Ferruh Yigit 2024-01-12 1:19 ` Chaoyong He 2024-01-12 10:06 ` Ferruh Yigit 2023-12-14 10:24 ` [PATCH 3/3] net/nfp: free domain ID in close interface Chaoyong He 2024-01-12 12:05 ` [PATCH 0/3] fix some problems of flower firmware Ferruh Yigit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).