Re: [PATCH net-next 2/3] net: page_pool: add the possibility to sync DMA memory for non-coherent devices

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Thu, Nov 14, 2019 at 10:43:09PM +0100, Jesper Dangaard Brouer wrote:
> On Thu, 14 Nov 2019 13:04:26 -0800
> "Jonathan Lemon" <jonathan.lemon@gmail.com> wrote:
> 
> > On 14 Nov 2019, at 12:42, Ilias Apalodimas wrote:
> > 
> > > Hi Jonathan,
> > >
> > > On Thu, Nov 14, 2019 at 12:27:40PM -0800, Jonathan Lemon wrote:  
> > >>
> > >>
> > >> On 14 Nov 2019, at 10:53, Ilias Apalodimas wrote:
> > >>  
> > >>> [...]  
> > >>>>> index 2cbcdbdec254..defbfd90ab46 100644
> > >>>>> --- a/include/net/page_pool.h
> > >>>>> +++ b/include/net/page_pool.h
> > >>>>> @@ -65,6 +65,9 @@ struct page_pool_params {
> > >>>>>  	int		nid;  /* Numa node id to allocate from pages from */
> > >>>>>  	struct device	*dev; /* device, for DMA pre-mapping purposes */
> > >>>>>  	enum dma_data_direction dma_dir; /* DMA mapping direction */
> > >>>>> +	unsigned int	max_len; /* max DMA sync memory size */
> > >>>>> +	unsigned int	offset;  /* DMA addr offset */
> > >>>>> +	u8 sync;
> > >>>>>  };  
> > >>>>
> > >>>> How about using PP_FLAG_DMA_SYNC instead of another flag word?
> > >>>> (then it can also be gated on having DMA_MAP enabled)  
> > >>>
> > >>> You mean instead of the u8?
> > >>> As you pointed out on your V2 comment of the mail, some cards don't 
> > >>> sync back to device.
> > >>>
> > >>> As the API tries to be generic a u8 was choosen instead of a flag
> > >>> to cover these use cases. So in time we'll change the semantics of
> > >>> this to 'always sync', 'dont sync if it's an skb-only queue' etc.
> > >>>
> > >>> The first case Lorenzo covered is sync the required len only instead 
> > >>> of the full buffer  
> > >>
> > >> Yes, I meant instead of:
> > >> +		.sync = 1,
> > >>
> > >> Something like:
> > >>         .flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC
> > >>
> 
> I actually agree and think we could use a flag. I suggest
> PP_FLAG_DMA_SYNC_DEV to indicate that this DMA-sync-for-device.
> 
> Ilias notice that the change I requested to Lorenzo, that dma_sync_size
> default value is 0xFFFFFFFF (-1).  That makes dma_sync_size==0 a valid
> value, which you can use in the cases, where you know that nobody have
> written into the data-area.  This allow us to selectively choose it for
> these cases.

Okay, then i guess the flag is a better fit for this.
The only difference would be that the sync semantics will be done on 'per
packet' basis,  instead of 'per pool', but that should be fine for our cases.

Cheers
/Ilias
> 
> -- 
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer
>