[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$>
>