On 8 Nov 2021, at 14:59, Lorenzo Bianconi wrote:
>> On Thu, 4 Nov 2021 18:35:33 +0100 Lorenzo Bianconi wrote:
>>> -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);
>>> + unsigned long base_len, copy_len, frag_off_total;
>>> + struct xdp_buff *xdp = (struct xdp_buff *)ctx;
>>> + struct skb_shared_info *sinfo;
>>> + int i;
>>> +
>>> + if (likely(!xdp_buff_is_mb(xdp))) {
>>
>> Would it be better to do
>>
>> if (xdp->data_end - xdp->data >= off + len)
>>
>> ?
>
> Hi Jakub,
>
> I am fine with the patch (just a typo inline), thx :)
> I will let Eelco to comment since he wrote the original code.
> If there is no objections, I will integrate it in v18.
Sorry for the late response, but both suggestions look fine to me.
>>
>>> + memcpy(dst_buff, xdp->data + off, len);
>>> + return 0;
>>> + }
>>> +
>>> + base_len = xdp->data_end - xdp->data;
>>> + frag_off_total = base_len;
>>> + sinfo = xdp_get_shared_info_from_buff(xdp);
>>> +
>>> + /* If we need to copy data from the base buffer do it */
>>> + if (off < base_len) {
>>> + copy_len = min(len, base_len - off);
>>> + memcpy(dst_buff, xdp->data + off, copy_len);
>>> +
>>> + off += copy_len;
>>> + len -= copy_len;
>>> + dst_buff += copy_len;
>>> + }
>>> +
>>> + /* Copy any remaining data from the fragments */
>>> + for (i = 0; len && i < sinfo->nr_frags; i++) {
>>> + skb_frag_t *frag = &sinfo->frags[i];
>>> + unsigned long frag_len, frag_off;
>>> +
>>> + frag_len = skb_frag_size(frag);
>>> + frag_off = off - frag_off_total;
>>> + if (frag_off < frag_len) {
>>> + copy_len = min(len, frag_len - frag_off);
>>> + memcpy(dst_buff,
>>> + skb_frag_address(frag) + frag_off, copy_len);
>>> +
>>> + off += copy_len;
>>> + len -= copy_len;
>>> + dst_buff += copy_len;
>>> + }
>>> + frag_off_total += frag_len;
>>> + }
>>> +
>>
>> nit: can't help but feel that you can merge base copy and frag copy:
>>
>> sinfo = xdp_get_shared_info_from_buff(xdp);
>> next_frag = &sinfo->frags[0];
>> end_frag = &sinfo->frags[sinfo->nr_frags];
>>
>> ptr_off = 0;
>> ptr_buf = xdp->data;
>> ptr_len = xdp->data_end - xdp->data;
>>
>> while (true) {
>> if (off < ptr_off + ptr_len) {
>> copy_off = ptr_off - off;
>
> I guess here should be:
> copy_off = off - ptr_off;
>
>> copy_len = min(len, ptr_len - copy_off);
>> memcpy(dst_buff, ptr_buf + copy_off, copy_len);
>>
>> off += copy_len;
>> len -= copy_len;
>> dst_buff += copy_len;
>> }
>>
>> if (!len || next_frag == end_frag)
>> break;
>>
>> ptr_off += ptr_len;
>> ptr_buf = skb_frag_address(next_frag);
>> ptr_len = skb_frag_size(next_frag);
>> next_frag++;
>> }
>>
>> Up to you.
