| Message ID | 20240123051245.3801246-1-rkannoth@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] octeontx2-af: Initialize bitmap arrays. | expand |
+ Suman Ghosh <sumang@marvell.com> On Tue, Jan 23, 2024 at 10:42:45AM +0530, Ratheesh Kannoth wrote: > kmalloc_array() does not initializes memory to zero. > This causes issues with bitmap. Use devm_kcalloc() > to fix the issue. Hi Ratheesh, I assume that the reason that the cited commit moved away from devm_ allocations was to allow more natural management of the resources independently of the life cycle of the driver instance. Or in other words, the need to free the bitmaps in npc_mcam_rsrcs_deinit() probably indicates that devm_ allocations of them aren't giving us anything. So, perhaps kcalloc() is more appropriate than devm_kcalloc() ? > Fixes: dd7842878633 ("octeontx2-af: Add new devlink param to configure maximum usable NIX block LFs") > Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com>
> From: Simon Horman <horms@kernel.org> > Subject: [EXT] Re: [PATCH net] octeontx2-af: Initialize bitmap arrays. > > Hi Ratheesh, > > I assume that the reason that the cited commit moved away from devm_ > allocations was to allow more natural management of the resources > independently of the life cycle of the driver instance. Or in other words, the > need to free the bitmaps in npc_mcam_rsrcs_deinit() probably indicates that > devm_ allocations of them aren't giving us anything. > > So, perhaps kcalloc() is more appropriate than devm_kcalloc() ? This was a comment from @Subbaraya Sundeep Bhatta during our internal review. Could you please help with below questions/doubts ? 1. why devm_kfree() API is available if it is done independently 2. I could see instances of devm_kfree() usage in current kernel where it does explicit calls. -Ratheesh
On Wed, Jan 24, 2024 at 03:01:15AM +0000, Ratheesh Kannoth wrote: > > From: Simon Horman <horms@kernel.org> > > Subject: [EXT] Re: [PATCH net] octeontx2-af: Initialize bitmap arrays. > > > > Hi Ratheesh, > > > > I assume that the reason that the cited commit moved away from devm_ > > allocations was to allow more natural management of the resources > > independently of the life cycle of the driver instance. Or in other words, the > > need to free the bitmaps in npc_mcam_rsrcs_deinit() probably indicates that > > devm_ allocations of them aren't giving us anything. > > > > So, perhaps kcalloc() is more appropriate than devm_kcalloc() ? > This was a comment from @Subbaraya Sundeep Bhatta during our internal review. > Could you please help with below questions/doubts ? > 1. why devm_kfree() API is available if it is done independently Hi Ratheesh, I think the question is: if the devm_kfree() calls are removed, then is the lifecycle of the objects in question managed correctly? > 2. I could see instances of devm_kfree() usage in current kernel where it does explicit calls. Sure. But in this case the use of devm_* doesn't seem to be adding anything if the memory is _always_ freed by explicit calls to devm_kfree().
> From: Simon Horman <horms@kernel.org> > Subject: Re: [EXT] Re: [PATCH net] octeontx2-af: Initialize bitmap arrays. > > I think the question is: if the devm_kfree() calls are removed, then is the > lifecycle of the objects in question managed correctly? If lifecycle of the objects are managed correctly without devm_kfree(), why this API is Provided and exported in kernel ? > > > 2. I could see instances of devm_kfree() usage in current kernel where it > does explicit calls. > > Sure. But in this case the use of devm_* doesn't seem to be adding anything > if the memory is _always_ freed by explicit calls to devm_kfree(). I got it. I would like to keep the diff minimal (rather than deleting lines diff). would this be okay ?
On Wed, Jan 24, 2024 at 10:43:26AM +0000, Ratheesh Kannoth wrote: > > From: Simon Horman <horms@kernel.org> > > Subject: Re: [EXT] Re: [PATCH net] octeontx2-af: Initialize bitmap arrays. > > > > I think the question is: if the devm_kfree() calls are removed, then is the > > lifecycle of the objects in question managed correctly? > If lifecycle of the objects are managed correctly without devm_kfree(), why this API is > Provided and exported in kernel ? When the lifecycle of an object is such that it is freed when the device is detached and at no other time, then devm_* can be helpful because there is no need for devm_free calls. I do understand that devm_free() exists, and there are cases where it makes sense. But I don't think devm_ is buying us anything here. > > > > > > 2. I could see instances of devm_kfree() usage in current kernel where it > > does explicit calls. > > > > Sure. But in this case the use of devm_* doesn't seem to be adding anything > > if the memory is _always_ freed by explicit calls to devm_kfree(). > I got it. I would like to keep the diff minimal (rather than deleting lines diff). would this be okay ? My feeling is that if you change your patch to: 1. Use kcalloc() instead of devm_kcalloc() 2. Not change kfree() calls to devm_kfree() Then you will end up with a smaller diff than the current patch. And it will address the problem described in the patch description.
On 1/22/2024 9:12 PM, Ratheesh Kannoth wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > kmalloc_array() does not initializes memory to zero. > This causes issues with bitmap. Use devm_kcalloc() > to fix the issue. > Is there any reason to not use: bitmap_zalloc() and bitmap_free()? > Fixes: dd7842878633 ("octeontx2-af: Add new devlink param to configure maximum usable NIX block LFs") > Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com> > --- > .../ethernet/marvell/octeontx2/af/rvu_npc.c | 55 ++++++++++--------- > 1 file changed, 28 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c > index 167145bdcb75..7539e6f0290a 100644 > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c > @@ -1850,13 +1850,13 @@ void npc_mcam_rsrcs_deinit(struct rvu *rvu) > { > struct npc_mcam *mcam = &rvu->hw->mcam; > > - kfree(mcam->bmap); > - kfree(mcam->bmap_reverse); > - kfree(mcam->entry2pfvf_map); > - kfree(mcam->cntr2pfvf_map); > - kfree(mcam->entry2cntr_map); > - kfree(mcam->cntr_refcnt); > - kfree(mcam->entry2target_pffunc); > + devm_kfree(rvu->dev, mcam->bmap); > + devm_kfree(rvu->dev, mcam->bmap_reverse); > + devm_kfree(rvu->dev, mcam->entry2pfvf_map); > + devm_kfree(rvu->dev, mcam->cntr2pfvf_map); > + devm_kfree(rvu->dev, mcam->entry2cntr_map); > + devm_kfree(rvu->dev, mcam->cntr_refcnt); > + devm_kfree(rvu->dev, mcam->entry2target_pffunc); > kfree(mcam->counters.bmap); > } > > @@ -1904,21 +1904,22 @@ int npc_mcam_rsrcs_init(struct rvu *rvu, int blkaddr) > mcam->pf_offset = mcam->nixlf_offset + nixlf_count; > > /* Allocate bitmaps for managing MCAM entries */ > - mcam->bmap = kmalloc_array(BITS_TO_LONGS(mcam->bmap_entries), > - sizeof(long), GFP_KERNEL); > + mcam->bmap = devm_kcalloc(rvu->dev, BITS_TO_LONGS(mcam->bmap_entries), > + sizeof(long), GFP_KERNEL); This is pretty much bitmap_zalloc(), except with devm. As Simon is asking, is devm really necessary? If not bitmap_zalloc() and bitmap_free() seem to fit your use well if I'm not mistaken. Thanks, Brett > if (!mcam->bmap) > return -ENOMEM; > > - mcam->bmap_reverse = kmalloc_array(BITS_TO_LONGS(mcam->bmap_entries), > - sizeof(long), GFP_KERNEL); > + mcam->bmap_reverse = devm_kcalloc(rvu->dev, > + BITS_TO_LONGS(mcam->bmap_entries), > + sizeof(long), GFP_KERNEL); > if (!mcam->bmap_reverse) > goto free_bmap; > > mcam->bmap_fcnt = mcam->bmap_entries; > > /* Alloc memory for saving entry to RVU PFFUNC allocation mapping */ > - mcam->entry2pfvf_map = kmalloc_array(mcam->bmap_entries, > - sizeof(u16), GFP_KERNEL); > + mcam->entry2pfvf_map = devm_kcalloc(rvu->dev, mcam->bmap_entries, > + sizeof(u16), GFP_KERNEL); > if (!mcam->entry2pfvf_map) > goto free_bmap_reverse; > > @@ -1941,27 +1942,27 @@ int npc_mcam_rsrcs_init(struct rvu *rvu, int blkaddr) > if (err) > goto free_entry_map; > > - mcam->cntr2pfvf_map = kmalloc_array(mcam->counters.max, > - sizeof(u16), GFP_KERNEL); > + mcam->cntr2pfvf_map = devm_kcalloc(rvu->dev, mcam->counters.max, > + sizeof(u16), GFP_KERNEL); > if (!mcam->cntr2pfvf_map) > goto free_cntr_bmap; > > /* Alloc memory for MCAM entry to counter mapping and for tracking > * counter's reference count. > */ > - mcam->entry2cntr_map = kmalloc_array(mcam->bmap_entries, > - sizeof(u16), GFP_KERNEL); > + mcam->entry2cntr_map = devm_kcalloc(rvu->dev, mcam->bmap_entries, > + sizeof(u16), GFP_KERNEL); > if (!mcam->entry2cntr_map) > goto free_cntr_map; > > - mcam->cntr_refcnt = kmalloc_array(mcam->counters.max, > - sizeof(u16), GFP_KERNEL); > + mcam->cntr_refcnt = devm_kcalloc(rvu->dev, mcam->counters.max, > + sizeof(u16), GFP_KERNEL); > if (!mcam->cntr_refcnt) > goto free_entry_cntr_map; > > /* Alloc memory for saving target device of mcam rule */ > - mcam->entry2target_pffunc = kmalloc_array(mcam->total_entries, > - sizeof(u16), GFP_KERNEL); > + mcam->entry2target_pffunc = devm_kcalloc(rvu->dev, mcam->total_entries, > + sizeof(u16), GFP_KERNEL); > if (!mcam->entry2target_pffunc) > goto free_cntr_refcnt; > > @@ -1978,19 +1979,19 @@ int npc_mcam_rsrcs_init(struct rvu *rvu, int blkaddr) > return 0; > > free_cntr_refcnt: > - kfree(mcam->cntr_refcnt); > + devm_kfree(rvu->dev, mcam->cntr_refcnt); > free_entry_cntr_map: > - kfree(mcam->entry2cntr_map); > + devm_kfree(rvu->dev, mcam->entry2cntr_map); > free_cntr_map: > - kfree(mcam->cntr2pfvf_map); > + devm_kfree(rvu->dev, mcam->cntr2pfvf_map); > free_cntr_bmap: > kfree(mcam->counters.bmap); > free_entry_map: > - kfree(mcam->entry2pfvf_map); > + devm_kfree(rvu->dev, mcam->entry2pfvf_map); > free_bmap_reverse: > - kfree(mcam->bmap_reverse); > + devm_kfree(rvu->dev, mcam->bmap_reverse); > free_bmap: > - kfree(mcam->bmap); > + devm_kfree(rvu->dev, mcam->bmap); > > return -ENOMEM; > } > -- > 2.25.1 > >
> From: Simon Horman <horms@kernel.org> > Subject: Re: [EXT] Re: [PATCH net] octeontx2-af: Initialize bitmap arrays. > I do understand that devm_free() exists, and there are cases where it makes > sense. But I don't think devm_ is buying us anything here. For graceful Error handling, this API makes sense. > 1. Use kcalloc() instead of devm_kcalloc() 2. Not change kfree() calls to > devm_kfree() > > Then you will end up with a smaller diff than the current patch. > And it will address the problem described in the patch description. ACK.
> From: Brett Creeley <bcreeley@amd.com> > Subject: [EXT] Re: [PATCH net] octeontx2-af: Initialize bitmap arrays. > Is there any reason to not use: > > bitmap_zalloc() and bitmap_free()? Will follow simon's suggestion to keep patch diff minimal. As bitmap_zalloc() does not give any advantage over the other. > > This is pretty much bitmap_zalloc(), except with devm. As Simon is asking, is > devm really necessary? Will use kcalloc.
On 1/24/2024 9:06 PM, Ratheesh Kannoth wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > >> From: Brett Creeley <bcreeley@amd.com> >> Subject: [EXT] Re: [PATCH net] octeontx2-af: Initialize bitmap arrays. >> Is there any reason to not use: >> >> bitmap_zalloc() and bitmap_free()? > Will follow simon's suggestion to keep patch diff minimal. As bitmap_zalloc() does not give any advantage over the other. It does make some sense because in multiple places you are open coding bitmap_zalloc()->bitmap_alloc() in multiple places. For example: mcam->bmap = kmalloc_array(BITS_TO_LONGS(mcam->bmap_entries), + sizeof(long), GFP_KERNEL | __GFP_ZERO); This is exactly what bitmap_zalloc()->bitmap_alloc() are doing. > >> >> This is pretty much bitmap_zalloc(), except with devm. As Simon is asking, is >> devm really necessary? > Will use kcalloc.
On Thu, Jan 25, 2024 at 07:56:22AM -0800, Brett Creeley wrote: > > > On 1/24/2024 9:06 PM, Ratheesh Kannoth wrote: > > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > > > > > From: Brett Creeley <bcreeley@amd.com> > > > Subject: [EXT] Re: [PATCH net] octeontx2-af: Initialize bitmap arrays. > > > Is there any reason to not use: > > > > > > bitmap_zalloc() and bitmap_free()? > > Will follow simon's suggestion to keep patch diff minimal. As bitmap_zalloc() does not give any advantage over the other. > > It does make some sense because in multiple places you are open coding > bitmap_zalloc()->bitmap_alloc() in multiple places. > > For example: > > mcam->bmap = kmalloc_array(BITS_TO_LONGS(mcam->bmap_entries), > + sizeof(long), GFP_KERNEL | __GFP_ZERO); > > This is exactly what bitmap_zalloc()->bitmap_alloc() are doing. Yes, I agree and I should have suggested using bitmap_zalloc() and bitmap_free().
> From: Simon Horman <horms@kernel.org> > Subject: Re: [EXT] Re: [PATCH net] octeontx2-af: Initialize bitmap arrays. > > mcam->bmap = kmalloc_array(BITS_TO_LONGS(mcam- > >bmap_entries), > > + sizeof(long), GFP_KERNEL | __GFP_ZERO); > > > > This is exactly what bitmap_zalloc()->bitmap_alloc() are doing. > > Yes, I agree and I should have suggested using > bitmap_zalloc() and bitmap_free(). > ACK.
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c index 167145bdcb75..7539e6f0290a 100644 --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c @@ -1850,13 +1850,13 @@ void npc_mcam_rsrcs_deinit(struct rvu *rvu) { struct npc_mcam *mcam = &rvu->hw->mcam; - kfree(mcam->bmap); - kfree(mcam->bmap_reverse); - kfree(mcam->entry2pfvf_map); - kfree(mcam->cntr2pfvf_map); - kfree(mcam->entry2cntr_map); - kfree(mcam->cntr_refcnt); - kfree(mcam->entry2target_pffunc); + devm_kfree(rvu->dev, mcam->bmap); + devm_kfree(rvu->dev, mcam->bmap_reverse); + devm_kfree(rvu->dev, mcam->entry2pfvf_map); + devm_kfree(rvu->dev, mcam->cntr2pfvf_map); + devm_kfree(rvu->dev, mcam->entry2cntr_map); + devm_kfree(rvu->dev, mcam->cntr_refcnt); + devm_kfree(rvu->dev, mcam->entry2target_pffunc); kfree(mcam->counters.bmap); } @@ -1904,21 +1904,22 @@ int npc_mcam_rsrcs_init(struct rvu *rvu, int blkaddr) mcam->pf_offset = mcam->nixlf_offset + nixlf_count; /* Allocate bitmaps for managing MCAM entries */ - mcam->bmap = kmalloc_array(BITS_TO_LONGS(mcam->bmap_entries), - sizeof(long), GFP_KERNEL); + mcam->bmap = devm_kcalloc(rvu->dev, BITS_TO_LONGS(mcam->bmap_entries), + sizeof(long), GFP_KERNEL); if (!mcam->bmap) return -ENOMEM; - mcam->bmap_reverse = kmalloc_array(BITS_TO_LONGS(mcam->bmap_entries), - sizeof(long), GFP_KERNEL); + mcam->bmap_reverse = devm_kcalloc(rvu->dev, + BITS_TO_LONGS(mcam->bmap_entries), + sizeof(long), GFP_KERNEL); if (!mcam->bmap_reverse) goto free_bmap; mcam->bmap_fcnt = mcam->bmap_entries; /* Alloc memory for saving entry to RVU PFFUNC allocation mapping */ - mcam->entry2pfvf_map = kmalloc_array(mcam->bmap_entries, - sizeof(u16), GFP_KERNEL); + mcam->entry2pfvf_map = devm_kcalloc(rvu->dev, mcam->bmap_entries, + sizeof(u16), GFP_KERNEL); if (!mcam->entry2pfvf_map) goto free_bmap_reverse; @@ -1941,27 +1942,27 @@ int npc_mcam_rsrcs_init(struct rvu *rvu, int blkaddr) if (err) goto free_entry_map; - mcam->cntr2pfvf_map = kmalloc_array(mcam->counters.max, - sizeof(u16), GFP_KERNEL); + mcam->cntr2pfvf_map = devm_kcalloc(rvu->dev, mcam->counters.max, + sizeof(u16), GFP_KERNEL); if (!mcam->cntr2pfvf_map) goto free_cntr_bmap; /* Alloc memory for MCAM entry to counter mapping and for tracking * counter's reference count. */ - mcam->entry2cntr_map = kmalloc_array(mcam->bmap_entries, - sizeof(u16), GFP_KERNEL); + mcam->entry2cntr_map = devm_kcalloc(rvu->dev, mcam->bmap_entries, + sizeof(u16), GFP_KERNEL); if (!mcam->entry2cntr_map) goto free_cntr_map; - mcam->cntr_refcnt = kmalloc_array(mcam->counters.max, - sizeof(u16), GFP_KERNEL); + mcam->cntr_refcnt = devm_kcalloc(rvu->dev, mcam->counters.max, + sizeof(u16), GFP_KERNEL); if (!mcam->cntr_refcnt) goto free_entry_cntr_map; /* Alloc memory for saving target device of mcam rule */ - mcam->entry2target_pffunc = kmalloc_array(mcam->total_entries, - sizeof(u16), GFP_KERNEL); + mcam->entry2target_pffunc = devm_kcalloc(rvu->dev, mcam->total_entries, + sizeof(u16), GFP_KERNEL); if (!mcam->entry2target_pffunc) goto free_cntr_refcnt; @@ -1978,19 +1979,19 @@ int npc_mcam_rsrcs_init(struct rvu *rvu, int blkaddr) return 0; free_cntr_refcnt: - kfree(mcam->cntr_refcnt); + devm_kfree(rvu->dev, mcam->cntr_refcnt); free_entry_cntr_map: - kfree(mcam->entry2cntr_map); + devm_kfree(rvu->dev, mcam->entry2cntr_map); free_cntr_map: - kfree(mcam->cntr2pfvf_map); + devm_kfree(rvu->dev, mcam->cntr2pfvf_map); free_cntr_bmap: kfree(mcam->counters.bmap); free_entry_map: - kfree(mcam->entry2pfvf_map); + devm_kfree(rvu->dev, mcam->entry2pfvf_map); free_bmap_reverse: - kfree(mcam->bmap_reverse); + devm_kfree(rvu->dev, mcam->bmap_reverse); free_bmap: - kfree(mcam->bmap); + devm_kfree(rvu->dev, mcam->bmap); return -ENOMEM; }
kmalloc_array() does not initializes memory to zero. This causes issues with bitmap. Use devm_kcalloc() to fix the issue. Fixes: dd7842878633 ("octeontx2-af: Add new devlink param to configure maximum usable NIX block LFs") Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com> --- .../ethernet/marvell/octeontx2/af/rvu_npc.c | 55 ++++++++++--------- 1 file changed, 28 insertions(+), 27 deletions(-)