Re: [PATCH v8 bpf-next 09/14] bpd: add multi-buffer support to xdp copy helpers

["Eelco Chaudron" <echaudro@redhat.com>]

On 8 Apr 2021, at 23:04, Vladimir Oltean wrote:

> On Thu, Apr 08, 2021 at 02:51:01PM +0200, Lorenzo Bianconi wrote:
>> From: Eelco Chaudron <echaudro@redhat.com>
>>
>> This patch adds support for multi-buffer for the following helpers:
>>   - bpf_xdp_output()
>>   - bpf_perf_event_output()
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>> ---
>
> Also there is a typo in the commit message: bpd -> bpf.

ACK, will fix in next version.

>>  net/core/filter.c                             |  63 ++++++++-
>>  .../selftests/bpf/prog_tests/xdp_bpf2bpf.c    | 127 
>> ++++++++++++------
>>  .../selftests/bpf/progs/test_xdp_bpf2bpf.c    |   3 +-
>>  3 files changed, 149 insertions(+), 44 deletions(-)
>>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index c4eb1392f88e..c00f52ab2532 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -4549,10 +4549,56 @@ static const struct bpf_func_proto 
>> bpf_sk_ancestor_cgroup_id_proto = {
>>  };
>>  #endif
>>
>> -static unsigned long bpf_xdp_copy(void *dst_buff, const void 
>> *src_buff,
>> +static unsigned long bpf_xdp_copy(void *dst_buff, const void *ctx,
>>  				  unsigned long off, unsigned long len)
>>  {
>> -	memcpy(dst_buff, src_buff + off, len);
>> +	struct xdp_buff *xdp = (struct xdp_buff *)ctx;
>
> There is no need to cast a void pointer in C.

I added this as the void pointer is a const. However, looking at it 
again, we should probably change xdp_get_shared_info_from_buff() to also 
take a const pointer, i.e.:

    static inline struct xdp_shared_info *
   -xdp_get_shared_info_from_buff(struct xdp_buff *xdp)
   +xdp_get_shared_info_from_buff(const struct xdp_buff *xdp)
    {
           BUILD_BUG_ON(sizeof(struct xdp_shared_info) >
                        sizeof(struct skb_shared_info));

What do you think Lorenzo?

>> +	struct xdp_shared_info *xdp_sinfo;
>> +	unsigned long base_len;
>> +
>> +	if (likely(!xdp->mb)) {
>> +		memcpy(dst_buff, xdp->data + off, len);
>> +		return 0;
>> +	}
>> +
>> +	base_len = xdp->data_end - xdp->data;
>
> Would a static inline int xdp_buff_head_len() be useful?

Guess everybody is using the xdp->data_end - xdp->data, in there code. 
But I guess we can add a static inline and change all code, but I 
don’t think we should do it as part of this patchset. I would also 
call it something like xdp_buff_data_len()?

>> +	xdp_sinfo = xdp_get_shared_info_from_buff(xdp);
>> +	do {
>> +		const void *src_buff = NULL;
>> +		unsigned long copy_len = 0;
>> +
>> +		if (off < base_len) {
>> +			src_buff = xdp->data + off;
>> +			copy_len = min(len, base_len - off);
>> +		} else {
>> +			unsigned long frag_off_total = base_len;
>> +			int i;
>> +
>> +			for (i = 0; i < xdp_sinfo->nr_frags; i++) {
>> +				skb_frag_t *frag = &xdp_sinfo->frags[i];
>> +				unsigned long frag_len, frag_off;
>> +
>> +				frag_len = xdp_get_frag_size(frag);
>> +				frag_off = off - frag_off_total;
>> +				if (frag_off < frag_len) {
>> +					src_buff = xdp_get_frag_address(frag) +
>> +						   frag_off;
>> +					copy_len = min(len,
>> +						       frag_len - frag_off);
>> +					break;
>> +				}
>> +				frag_off_total += frag_len;
>> +			}
>> +		}
>> +		if (!src_buff)
>> +			break;
>> +
>> +		memcpy(dst_buff, src_buff, copy_len);
>> +		off += copy_len;
>> +		len -= copy_len;
>> +		dst_buff += copy_len;
>> +	} while (len);
>> +
>>  	return 0;
>>  }