Re: [PATCH net 1/6] xsk: respect tailroom for ZC setups

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

Stanislav Fomichev <stfomichev@gmail.com> writes:

> On 03/16, Maciej Fijalkowski wrote:
>> Multi-buffer XDP stores information about frags in skb_shared_info that
>> sits at the tailroom of a packet. The storage space is reserved via
>> xdp_data_hard_end():
>> 
>> 	((xdp)->data_hard_start + (xdp)->frame_sz -	\
>> 	 SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
>> 
>> and then we refer to it via macro below:
>> 
>> static inline struct skb_shared_info *
>> xdp_get_shared_info_from_buff(const struct xdp_buff *xdp)
>> {
>>         return (struct skb_shared_info *)xdp_data_hard_end(xdp);
>> }
>> 
>> Currently we do not respect this tailroom space in multi-buffer AF_XDP
>> ZC scenario. To address this, introduce xsk_pool_get_tailroom() and use
>> it within xsk_pool_get_rx_frame_size() which is used in ZC drivers to
>> configure length of HW Rx buffer.
>> 
>> xsk_pool_get_tailroom() is only reserving necessary space when pool is
>> zc and underlying netdev supports zc multi-buffer. Since this function
>> relies on pool->umem->zc setting, set it before ndo_bpf during zc
>> configuration, so that driver that actually calls
>> xsk_pool_get_rx_frame_size() inside ndo_bpf will get correct tailroom
>> value.
>> 
>> Fixes: 24ea50127ecf ("xsk: support mbuf on ZC RX")
>> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>> ---
>>  include/net/xdp_sock_drv.h | 21 ++++++++++++++++++++-
>>  net/xdp/xsk_buff_pool.c    |  3 ++-
>>  2 files changed, 22 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
>> index 6b9ebae2dc95..13b2aae00737 100644
>> --- a/include/net/xdp_sock_drv.h
>> +++ b/include/net/xdp_sock_drv.h
>> @@ -41,6 +41,19 @@ static inline u32 xsk_pool_get_headroom(struct xsk_buff_pool *pool)
>>  	return XDP_PACKET_HEADROOM + pool->headroom;
>>  }
>>  
>> +static inline u32 xsk_pool_get_tailroom(struct xsk_buff_pool *pool)
>> +{
>> +	struct xdp_umem *umem = pool->umem;
>> +
>> +	/* Reserve tailroom only for zero-copy pools that opted into
>> +	 * multi-buffer. The reserved area is used for skb_shared_info,
>> +	 * matching the XDP core's xdp_data_hard_end() layout.
>> +	 */
>> +	if (umem->zc && (umem->flags & XDP_UMEM_SG_FLAG))
>> +		return SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>> +	return 0;
>> +}
>> +
>>  static inline u32 xsk_pool_get_chunk_size(struct xsk_buff_pool *pool)
>>  {
>>  	return pool->chunk_size;
>> @@ -48,7 +61,8 @@ static inline u32 xsk_pool_get_chunk_size(struct xsk_buff_pool *pool)
>>  
>>  static inline u32 xsk_pool_get_rx_frame_size(struct xsk_buff_pool *pool)
>>  {
>> -	return xsk_pool_get_chunk_size(pool) - xsk_pool_get_headroom(pool);
>> +	return xsk_pool_get_chunk_size(pool) - xsk_pool_get_headroom(pool) -
>> +	       xsk_pool_get_tailroom(pool);
>>  }
>>  
>>  static inline u32 xsk_pool_get_rx_frag_step(struct xsk_buff_pool *pool)
>> @@ -332,6 +346,11 @@ static inline u32 xsk_pool_get_headroom(struct xsk_buff_pool *pool)
>>  	return 0;
>>  }
>
> [..]
>  
>> +static inline u32 xsk_pool_get_tailroom(struct xsk_buff_pool *pool)
>> +{
>> +	return 0;
>> +}
>
> Not sure it's needed? xsk_pool_get_tailroom is only used by
> CONFIG_XDP_SOCKETS' version of xsk_pool_get_rx_frame_size.
>
>>  static inline u32 xsk_pool_get_chunk_size(struct xsk_buff_pool *pool)
>>  {
>>  	return 0;
>> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
>> index 37b7a68b89b3..2cfc19e363e3 100644
>> --- a/net/xdp/xsk_buff_pool.c
>> +++ b/net/xdp/xsk_buff_pool.c
>> @@ -213,6 +213,7 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
>>  	bpf.command = XDP_SETUP_XSK_POOL;
>>  	bpf.xsk.pool = pool;
>>  	bpf.xsk.queue_id = queue_id;
>> +	pool->umem->zc = true;
>>  
>>  	netdev_ops_assert_locked(netdev);
>>  	err = netdev->netdev_ops->ndo_bpf(netdev, &bpf);
>> @@ -224,13 +225,13 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
>>  		err = -EINVAL;
>>  		goto err_unreg_xsk;
>>  	}
>> -	pool->umem->zc = true;
>>  	pool->xdp_zc_max_segs = netdev->xdp_zc_max_segs;
>>  	return 0;
>>  
>>  err_unreg_xsk:
>>  	xp_disable_drv_zc(pool);
>>  err_unreg_pool:
>> +	pool->umem->zc = false;
>>  	if (!force_zc)
>>  		err = 0; /* fallback to copy mode */
>>  	if (err) {
>
> I'm not super familiar with the shared umem patch, but is it safe to
> unconditionally undo pool->umem->zc = false here? xp_assign_dev_shared
> looks at this umem->zc flag.. Presumably other places do as well on
> teardown?

Good catch!

I can elaborate a bit; the zero-copy property of umem is shared between
all users (sockets) of that umem. IOW, all sockets sharing an umem,
inherits whatever the first socket negotiated.

So, we could get into something like:

1. Socket A binds queue 0, ndo_bpf OK (umem->zc = true)
2. Socket B binds queue 1 via xp_assign_dev_shared()
   reads umem->zc == true, so flags = XDP_ZEROCOPY
   xp_assign_dev() sets umem->zc = true
   ndo_bpf() NOK for queue 1 -> error path: umem->zc = false (oops)
3. Socket A is still active on queue 0 in ZC mode, but umem->zc is now
   false

...and we'll have a bunch of checks on umem->zc that now has incorrect
state.

From this follows that the zc flag shouldn't be toggled on a shared
resource without checking if other consumers exist. I think a per-pool
zc flag is needed here or smth. :-(


Björn