Re: [PATCH net-next v1 03/11] xdp: Copy buffers with restricted lifetime

[Stanislav Fomichev]

On 11/11, Björn Töpel wrote:
> Stanislav Fomichev <sdf.kernel@xxxxxxxxx> writes:
> 
> > On 11/10, Björn Töpel wrote:
> 
> >> diff --git a/include/net/xdp.h b/include/net/xdp.h
> >> index 07231adfb5f8..1105bb9e3292 100644
> >> --- a/include/net/xdp.h
> >> +++ b/include/net/xdp.h
> >> @@ -81,6 +81,16 @@ enum xdp_buff_flags {
> >>  	 * XDP program is not attached.
> >>  	 */
> >>  	XDP_FLAGS_FRAGS_UNREADABLE	= BIT(2),
> >> +	/* Internal: the head netmem and the shared info are in &xdp_buff.ext.
> >> +	 * Not copied to xdp_frames or skbs.
> >> +	 */
> >> +	XDP_FLAGS_EXT			= BIT(3),
> >> +};
> >> +
> >> +/* For a head buffer that cannot hold kernel data, see xdp_init_buff_netmem() */
> >> +struct xdp_buff_ext {
> >> +	netmem_ref netmem;
> >
> > nit: netmem -> head ? and the same for xdp_init_buff_netmem netmem arg?
> 
> Yup, will fix.
> 
> >> @@ -138,7 +153,7 @@ static __always_inline void xdp_buff_set_frag_unreadable(struct xdp_buff *xdp)
> >>  
> >>  static __always_inline u32 xdp_buff_get_skb_flags(const struct xdp_buff *xdp)
> >>  {
> >> -	return xdp->flags;
> >> +	return xdp->flags & ~XDP_FLAGS_EXT;
> >
> > The only consumer is xdp_update_skb_frags_info, so probably not worth
> > filtering out XDP_FLAGS_EXT here? Or is it for
> > xdp_update_frame_from_buff? Can we explicitly clear it at the caller? 
> 
> It was for xdp_update_frame_from_buff(), but looking closer EXT never
> gets there...
> 
> I'll drop the mask. Thanks!
> 
> >> diff --git a/net/core/devmem.c b/net/core/devmem.c
> >> index 8f190b574beb..dea4a4a17e86 100644
> >> --- a/net/core/devmem.c
> >> +++ b/net/core/devmem.c
> >> @@ -698,5 +698,5 @@ static const struct memory_provider_ops dmabuf_devmem_ops = {
> >>  	.release_netmem		= mp_dmabuf_devmem_release_page,
> >>  	.nl_fill		= mp_dmabuf_devmem_nl_fill,
> >>  	.uninstall		= mp_dmabuf_devmem_uninstall,
> >> -	.caps			= MP_CAP_FRAG,
> >> +	.caps			= MP_CAP_FRAG | MP_CAP_RETAIN,
> >
> > Not sure I understand this part, can you explain?
> > These are MP_CAP_RETAIN because they are unreadable?
> 
> No, RETAIN is about lifetime: the stack may hold page pool references
> after the RX poll, e.g. skbs on a TCP receive queue. That's how devmem
> and zcrx work; without it the stack would try to copy them, and they'd
> trip the DEBUG_NET check in skb_add_rx_frag_netmem().
> 
> A provider without RETAIN gets copied, so it must be readable. I'll try
> to make that more clear in the kdoc, and probably refuse a provider that
> is neither.

Ah, yes, thanks, I did miss that warn in skb_add_rx_frag_netme, makes sense!