Re: [EXTERNAL] Re: [PATCH net 1/3] net: ti: icssg-prueth: Use page_pool API for RX buffer allocation

[Malladi, Meghana]

On 2/5/2025 11:11 PM, Ido Schimmel wrote:
> On Tue, Feb 04, 2025 at 11: 25: 02PM +0530, Malladi, Meghana wrote: > 
> Seems like none of the pages which have been allocated aren't getting > 
> recycled in the rx path after being used unless its some error case. 
> Will > try to fix this. 
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source 
> of this email and know the content is safe.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK! 
> v9dnXdhkXiNQgIoLtH6jcbhWBIydfvayMZ6bf68taZCHXfcLg8XIOscUa_XNxqzQWA$>
> ZjQcmQRYFpfptBannerEnd
>
> On Tue, Feb 04, 2025 at 11:25:02PM +0530, Malladi, Meghana wrote:
> > Seems like none of the pages which have been allocated aren't getting
> > recycled in the rx path after being used unless its some error case. Will
> > try to fix this.
>
> skb_mark_for_recycle() should help with page recycling when an skb that
> uses them is freed.
>
> Anyway, I believe that I don't see put call when tearing down the Rx
> ring because prueth_rx_cleanup() is using page_pool_recycle_direct()
> when it shouldn't. AFAICT, prueth_rx_cleanup() is only called from the
> control path (upon ndo_stop()) and not in NAPI context.

Ok I will use skb_mark_for_recycle()/page_pool_recycle_direct() 
accordingly to recycle the pages.

> > Also I have noticed, in prueth_prepare_rx_chan() pages are allocated per
> > number of descriptors for a channel, but they are not being used when a
> > packet is being recieved (in emac_rx_packet()) and rather new page is
> > allocated for the next upcoming packet. Is this a valid design, what are
> > your thoughts on this ?
>
> The new page is possibly a page that was recycled into the pool when a
> previous packet was freed / dropped.
>
> [...]
>
> > Yes I will add PP_FLAG_DMA_SYNC_DEV as well.
> > I believe page_pool_dma_sync_for_cpu() needs to be called sync Rx page for
> > CPU, am I right ? If so can you tell me, in what all cases should I call
> > this function.
>
> Before accessing the packet data.

Ok, thanks.

> > https://urldefense.com/v3/__https://lkml.iu.edu/hypermail/linux/ 
> kernel/2312.1/06353.html__;!!G3vK!R- 
> autrVAgf5rAbl3CYoqlN5gRE_NqPqYRg1NHkJ405Q33b6uKiHFI73PeRky46dBYBWQpmFThUyD$ <https://urldefense.com/v3/__https://lkml.iu.edu/hypermail/linux/kernel/2312.1/06353.html__;!!G3vK!R-autrVAgf5rAbl3CYoqlN5gRE_NqPqYRg1NHkJ405Q33b6uKiHFI73PeRky46dBYBWQpmFThUyD$>
> > In the above link it is quoted - "Note that this version performs DMA sync
> > unconditionally, even if the associated PP doesn't perform sync-for-device"
> > for the page_pool_dma_sync_for_cpu() function. So does that mean if I am
> > using this function I don't need explicily sync for device call?
>
> It's explained in the page pool documentation:
>
> "Driver is always responsible for syncing the pages for the CPU. Drivers
> may choose to take care of syncing for the device as well or set the
> PP_FLAG_DMA_SYNC_DEV flag to request that pages allocated from the page
> pool are already synced for the device."
>
> https://urldefense.com/v3/__https://docs.kernel.org/networking/ 
> page_pool.html*dma-sync__;Iw!!G3vK!R- 
> autrVAgf5rAbl3CYoqlN5gRE_NqPqYRg1NHkJ405Q33b6uKiHFI73PeRky46dBYBWQphNIm6Qm$ <https://urldefense.com/v3/__https://docs.kernel.org/networking/page_pool.html*dma-sync__;Iw!!G3vK!R-autrVAgf5rAbl3CYoqlN5gRE_NqPqYRg1NHkJ405Q33b6uKiHFI73PeRky46dBYBWQphNIm6Qm$>