Message-ID: <20200906133617.GC2785@lore-desk>
Date:   Sun, 6 Sep 2020 15:36:17 +0200
From:   Lorenzo Bianconi <lorenzo@...nel.org>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, davem@...emloft.net,
        lorenzo.bianconi@...hat.com, brouer@...hat.com,
        echaudro@...hat.com, sameehj@...zon.com, kuba@...nel.org,
        daniel@...earbox.net, ast@...nel.org, shayagr@...zon.com,
        edumazet@...gle.com
Subject: Re: [PATCH v2 net-next 6/9] bpf: helpers: add
 bpf_xdp_adjust_mb_header helper

> Lorenzo Bianconi wrote:
> > > Lorenzo Bianconi wrote:

[...]

> > > > + *	Description
> > > > + *		Adjust frame headers moving *offset* bytes from/to the second
> > > > + *		buffer to/from the first one. This helper can be used to move
> > > > + *		headers when the hw DMA SG does not copy all the headers in
> > > > + *		the first fragment.
> > 
> > + Eric to the discussion
> > 
> > > 
> > > This is confusing to read. Does this mean I can "move bytes to the second
> > > buffer from the first one" or "move bytes from the second buffer to the first
> > > one" And what are frame headers? I'm sure I can read below code and work
> > > it out, but users reading the header file should be able to parse this.
> > 
> > Our main goal with this helper is to fix the use-case where we request the hw
> > to put L2/L3/L4 headers (and all the TCP options) in the first fragment and TCP
> > data starting from the second fragment (headers split) but for some reasons
> > the hw puts the TCP options in the second fragment (if we understood correctly
> > this issue has been introduced by Eric @ NetDevConf 0x14).
> > bpf_xdp_adjust_mb_header() can fix this use-case moving bytes from the second fragment
> > to the first one (offset > 0) or from the first buffer to the second one (offset < 0).
> 
> Ah OK, so the description needs the information about how to use offset then it
> would have been clear I think. Something like that last line "moving bytes from
> the second fragment ...."

ack, right. I will do in v3.

> 
> So this is to fixup header-spit for RX zerocopy? Add that to the commit
> message then.

Right. I will improve comments in v3.

> 
> > 
> > > 
> > > Also we want to be able to read all data not just headers. Reading the
> > > payload of a TCP packet is equally important for many l7 load balancers.
> > > 
> > 
> > In order to avoid to slow down performances we require that eBPF sandbox can
> > read/write only buff0 in a xdp multi-buffer. The xdp program can only
> > perform some restricted changes to buff<n> (n >= 1) (e.g. what we did in
> > bpf_xdp_adjust_mb_header()).
> > You can find the xdp multi-buff design principles here [0][1]
> > 
> > [0] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
> > [1] http://people.redhat.com/lbiancon/conference/NetDevConf2020-0x14/add-xdp-on-driver.html - XDP multi-buffers section (slide 40)
> > 
> > > > + *
> > > > + *		A call to this helper is susceptible to change the underlying
> > > > + *		packet buffer. Therefore, at load time, all checks on pointers
> > > > + *		previously done by the verifier are invalidated and must be
> > > > + *		performed again, if the helper is used in combination with
> > > > + *		direct packet access.
> > > > + *
> > > > + *	Return
> > > > + *		0 on success, or a negative error in case of failure.
> > > >   */
> > > >  #define __BPF_FUNC_MAPPER(FN)		\
> > > >  	FN(unspec),			\
> > > > @@ -3727,6 +3741,7 @@ union bpf_attr {
> > > >  	FN(inode_storage_delete),	\
> > > >  	FN(d_path),			\
> > > >  	FN(copy_from_user),		\
> > > > +	FN(xdp_adjust_mb_header),	\
> > > >  	/* */
> > > >  
> > > >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > > index 47eef9a0be6a..ae6b10cf062d 100644
> > > > --- a/net/core/filter.c
> > > > +++ b/net/core/filter.c
> > > > @@ -3475,6 +3475,57 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
> > > >  	.arg2_type	= ARG_ANYTHING,
> > > >  };
> > > >  
> > > > +BPF_CALL_2(bpf_xdp_adjust_mb_header, struct  xdp_buff *, xdp,
> > > > +	   int, offset)
> > > > +{
> > > > +	void *data_hard_end, *data_end;
> > > > +	struct skb_shared_info *sinfo;
> > > > +	int frag_offset, frag_len;
> > > > +	u8 *addr;
> > > > +
> > > > +	if (!xdp->mb)
> > > > +		return -EOPNOTSUPP;
> 
> Not required for this patch necessarily but I think it would be better user
> experience if instead of EOPNOTSUPP here we did the header split. This
> would allocate a frag and copy the bytes around as needed. Yes it might
> be slow if you don't have a frag free in the driver, but if user wants to
> do header split and their hardware can't do it we would have a way out.
> 
> I guess it could be an improvement for later though.

I have no a strong opinion on this, I did it in this way to respect the rule "we
do not allocate memory for XDP".

@Jesper, David: thoughts?

> 
> 
> > > > +
> > > > +	sinfo = xdp_get_shared_info_from_buff(xdp);
> > > > +
> > > > +	frag_len = skb_frag_size(&sinfo->frags[0]);
> > > > +	if (offset > frag_len)
> > > > +		return -EINVAL;
> > > 
> > > What if we want data in frags[1] and so on.
> > > 
> > > > +
> > > > +	frag_offset = skb_frag_off(&sinfo->frags[0]);
> > > > +	data_end = xdp->data_end + offset;
> > > > +
> > > > +	if (offset < 0 && (-offset > frag_offset ||
> > > > +			   data_end < xdp->data + ETH_HLEN))
> > > > +		return -EINVAL;
> > > > +
> > > > +	data_hard_end = xdp_data_hard_end(xdp); /* use xdp->frame_sz */
> > > > +	if (data_end > data_hard_end)
> > > > +		return -EINVAL;
> > > > +
> > > > +	addr = page_address(skb_frag_page(&sinfo->frags[0])) + frag_offset;
> > > > +	if (offset > 0) {
> > > > +		memcpy(xdp->data_end, addr, offset);
> > > 
> > > But this could get expensive for large amount of data? And also
> > > limiting because we require the room to do the copy. Presumably
> > > the reason we have fargs[1] is because the normal page or half
> > > page is in use?
> > > 
> > > > +	} else {
> > > > +		memcpy(addr + offset, xdp->data_end + offset, -offset);
> > > > +		memset(xdp->data_end + offset, 0, -offset);
> > > > +	}
> > > > +
> > > > +	skb_frag_size_sub(&sinfo->frags[0], offset);
> > > > +	skb_frag_off_add(&sinfo->frags[0], offset);
> > > > +	xdp->data_end = data_end;
> > > > +
> > > > +	return 0;
> > > > +}
> > > 
> > > So overall I don't see the point of copying bytes from one frag to
> > > another. Create an API that adjusts the data pointers and then
> > > copies are avoided and manipulating frags is not needed.
> > 
> > please see above.
> 
> OK it makes more sense with the context. It doesn't have much if anything
> to do about making data visible to the BPF program. This is about
> changing the layout of the frags list.

correct.

> 
> How/when does the header split go wrong on the mvneta device? I guess
> this is to fix a real bug/issue not some theoritical one? An example
> in the commit message would make this concrete. Soemthing like,
> "When using RX zerocopy to mmap data into userspace application if
> a packet with [all these wild headers] is received rx zerocopy breaks
> because header split puts headers X in the data frag confusing apps".

This issue does not occur with mvneta since the driver is not capable of
performing header split AFAIK. The helper has been introduced to cover the
"issue" reported by Eric in his NetDevConf presentation. In order to test the
helper I modified the mventa rx napi loop in a controlled way (this patch can't
be sent upstream, it is for testing only :))
I will improve commit message in v3.

> 
> > 
> > > 
> > > Also and even more concerning I think this API requires the
> > > driver to populate shinfo. If we use TX_REDIRECT a lot or TX_XMIT
> > > this means we need to populate shinfo when its probably not ever
> > > used. If our driver is smart L2/L3 headers are in the readable
> > > data and prefetched. Writing frags into the end of a page is likely
> > > not free.
> > 
> > Sorry I did not get what you mean with "populate shinfo" here. We need to
> > properly set shared_info in order to create the xdp multi-buff.
> > Apart of header splits, please consider the main uses-cases for
> > xdp multi-buff are XDP with TSO and Jumbo frames.
> 
> The use case I have in mind is a XDP_TX or XDP_REDIRECT load balancer.
> I wont know this at the driver level and now I'll have to write into
> the back of every page with this shinfo just in case. If header
> split is working I should never need to even touch the page outside
> the first N bytes that were DMAd and in cache with DDIO. So its extra
> overhead for something that is unlikely to happen in the LB case.

So far the skb_shared_info in constructed in mvneta only if the hw splits
the received data in multiple buffers (so if the MTU is greater than 1 PAGE,
please see comments below). Moreover the shared_info is present only in the
first buffer.

> 
> If you take the simplest possible program that just returns XDP_TX
> and run a pkt generator against it. I believe (haven't run any
> tests) that you will see overhead now just from populating this
> shinfo. I think it needs to only be done when its needed e.g. when
> user makes this helper call or we need to build the skb and populate
> the frags there.

sure, I will carry out some tests.

> 
> I think a smart driver will just keep the frags list in whatever
> form it has them (rx descriptors?) and push them over to the
> tx descriptors without having to do extra work with frag lists.

I think there are many use-cases where we want to have this info available in
xdp_buff/xdp_frame. E.g: let's consider the following Jumbo frame example:
- MTU > 1 PAGE (so we the driver will split the received data in multiple rx
  descriptors)
- the driver performs a XDP_REDIRECT to a veth or cpumap

Relying on the proposed architecture we could enable GRO in veth or cpumap I
guess since we can build a non-linear skb from the xdp multi-buff, right?

> 
> > 
> > > 
> > > Did you benchmark this?
> > 
> > will do, I need to understand if we can use tiny buffers in mvneta.
> 
> Why tiny buffers? How does mvneta layout the frags when doing
> header split? Can we just benchmark what mvneta is doing at the
> end of this patch series?

for the moment mvneta can split the received data when the previous buffer is
full (e.g. when we the first page is completely written). I want to explore if
I can set a tiny buffer (e.g. 128B) as max received buffer to run some performance
tests and have some "comparable" results respect to the ones I got when I added XDP
support to mvneta.

> 
> Also can you try the basic XDP_TX case mentioned above.
> I don't want this to degrade existing use cases if at all
> possible.

sure, will do.

> 
> > 
> > > 
> > > In general users of this API should know the bytes they want
> > > to fetch. Use an API like this,
> > > 
> > >   bpf_xdp_adjust_bytes(xdp, start, end)
> > > 
> > > Where xdp is the frame, start is the first byte the user wants
> > > and end is the last byte. Then usually you can skip the entire
> > > copy part and just move the xdp pointesr around. The ugly case
> > > is if the user puts start/end across a frag boundary and a copy
> > > is needed. In that case maybe we use end as a hint and not a
> > > hard requirement.
> > > 
> > > The use case I see is I read L2/L3/L4 headers and I need the
> > > first N bytes of the payload. I know where the payload starts
> > > and I know how many bytes I need so,
> > > 
> > >   bpf_xdp_adjust_bytes(xdp, payload_offset, bytes_needed);
> > > 
> > > Then hopefully that is all in one frag. If its not I'll need
> > > to do a second helper call. Still nicer than forcing drivers
> > > to populate this shinfo I think. If you think I'm wrong try
> > > a benchmark. Benchmarks aside I get stuck when data_end and
> > > data_hard_end are too close together.
> > 
> > IIUC what you mean here is to expose L2/L3/L4 headers + some data to
> > the ebpf program to perform like L7 load-balancing, right?
> 
> Correct, but with extra context I see in this patch you are trying
> to build an XDP controlled header split. This seems like a different
> use case from mine.

I agree.

> 
> > Let's consider the Jumbo frames use-case (so the data are split in multiple
> > buffers). I can see to issues here:
> > - since in XDP we can't linearize the buffer if start and end are on the
> >   different pages (like we do in bpf_msg_pull_data()), are we ending up
> >   in the case where requested data are all in buff0? 
> 
> In this case I would expect either the helper returns how many bytes
> were pulled in, maybe just (start, end_of_frag) or user can find
> it from data_end pointer. Here end is just a hint.
> 
> > - if  start and end are in buff<2>, we should report the fragment number to the
> >   ebpf program to "fetch" the data. Are we exposing too low-level details to
> >   user-space?
> 
> Why do you need the frag number? Just say I want bytes (X,Y) if that
> happens to be on buff<2> let the helper find it.
> 
> I think having a helper to read/write any bytes is important and
> necessary, but the helper implemented in this patch is something
> else. I get naming is hard what if we called it xdp_header_split().

ack, sure. I will fix it in v3.

Regards,
Lorenzo

> 
> > 
> > Regards,
> > Lorenzo
> > 
> > > 
> > > Thanks,
> > > John
> 
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists