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

["Björn Töpel" <bjorn@kernel.org>]

Stanislav Fomichev <sdf.kernel@gmail.com> 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.

>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index 1d679e8fd649..0cf46d492d91 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -23,6 +23,8 @@
>>  #include <trace/events/xdp.h>
>>  #include <net/xdp_sock_drv.h>
>>  
>> +#include "dev.h"
>> +
>>  #define REG_STATE_NEW		0x0
>>  #define REG_STATE_REGISTERED	0x1
>>  #define REG_STATE_UNREGISTERED	0x2
>> @@ -559,10 +561,41 @@ void xdp_return_buff(struct xdp_buff *xdp)
>>  			     xdp->rxq->mem.type, true, xdp);
>>  
>>  out:
>> -	__xdp_return(virt_to_netmem(xdp->data), xdp->rxq->mem.type, true, xdp);
>> +	__xdp_return(xdp_buff_head_netmem(xdp), xdp->rxq->mem.type, true, xdp);
>>  }
>>  EXPORT_SYMBOL_GPL(xdp_return_buff);
>>  
>> +bool __xdp_buff_needs_copy(const struct xdp_buff *xdp)
>> +{
>> +	const struct skb_shared_info *sinfo;
>> +	u32 i;
>> +
>> +	/* Provider buffers come only from page pool queues */
>> +	if (xdp->rxq->mem.type != MEM_TYPE_PAGE_POOL)
>> +		return false;
>> +	if (!netmem_can_retain(xdp_buff_head_netmem(xdp)))
>> +		return true;
>> +	if (likely(!xdp_buff_has_frags(xdp)))
>> +		return false;
>> +
>> +	sinfo = xdp_get_shared_info_from_buff(xdp);
>> +	for (i = 0; i < sinfo->nr_frags; i++) {
>> +		if (!netmem_can_retain(skb_frag_netmem(&sinfo->frags[i])))
>> +			return true;
>
> Do we want/need to go that deep? Can we have a skb-like assumptions that
> mixing retain and non-retain in the same shinfo is not allowed?

Yep, let's make that the rule. I'll remove the deep checks.


Björn