Re: [PATCH ipsec-next v8 10/16] xfrm: iptfs: add fragmenting of larger than MTU user packets

[Sabrina Dubroca <sd@queasysnail.net>]

2024-08-08, 07:30:13 -0400, Christian Hopps wrote:
> 
> Sabrina Dubroca <sd@queasysnail.net> writes:
> 
> > 2024-08-06, 04:54:53 -0400, Christian Hopps wrote:
> > > 
> > > Sabrina Dubroca <sd@queasysnail.net> writes:
> > > 
> > > > 2024-08-04, 22:33:05 -0400, Christian Hopps wrote:
> > > > > > > +/* 1) skb->head should be cache aligned.
> > > > > > > + * 2) when resv is for L2 headers (i.e., ethernet) we want the cacheline to
> > > > > > > + * start -16 from data.
> > > > > > > + * 3) when resv is for L3+L2 headers IOW skb->data points at the IPTFS payload
> > > > > > > + * we want data to be cache line aligned so all the pushed headers will be in
> > > > > > > + * another cacheline.
> > > > > > > + */
> > > > > > > +#define XFRM_IPTFS_MIN_L3HEADROOM 128
> > > > > > > +#define XFRM_IPTFS_MIN_L2HEADROOM (64 + 16)
> > > > > >
> > > > > > How did you pick those values?
> > > > >
> > > > > That's what the comment is talking to. When reserving space for L2 headers we
> > > > > pick 64 + 16 (a 2^(<=6) cacheline + 16 bytes so the the cacheline should start
> > > > > -16 from where skb->data will point at.
> > > >
> > > > Hard-coding the x86 cacheline size is not a good idea. And what's the
> > > > 16B for? You don't know that it's enough for the actual L2 headers.
> > > 
> > > I am not hard coding the x86 cacheline. I am picking 64 as the largest cacheline that this is optimized for, it also works for smaller cachelines.
> > 
> > At least use SMP_CACHE_BYTES then?
> 
> Right, I have changed this work with L1_CACHE_BYTES value.
> 
> > > 16B is to allow for the incredibly common 14B L2 header to fit.
> > 
> > Why not use skb->dev->needed_headroom, like a bunch of tunnels are
> > already doing? No guessing required. ethernet is the most common, but
> > there's no reason to penalize other protocols when the information is
> > available.
> 
> We can't use `skb->dev->needed_headroom`, b/c `skb->dev` is not
> correct for the new packets. `skb->dev` is from the received IPTFS
> tunnel packet. The skb being created here are the inner user packets
> leaving the tunnel, so they have an L3 header (thus why we are only
> making room for L2 header). They are being handed to gro receive and
> still have to be routed to their correct destination interface/dev.

You're talking about RX now. You're assuming the main use-case is an
IPsec GW that's going to send the decapped packets out on another
ethernet interface? (or at least, that's that's a use-case worth
optimizing for)

What about TX? Is skb->dev->needed_headroom also incorrect there?

Is iptfs_alloc_skb's l3resv argument equivalent to a RX/TX switch?

> 16 handles the general common case an ethernet device being the
> destination, if it's not correct after routing, nothing is broken,
> it just means that we may or may not achieve this maximal cache
> locality (but we still might e.g., if its destined to a GRE tunnel
> then we are looking at a bunch more headers so they and the existing
> L3 header will occupy 2 cachelines anyway). Again, this is a best
> effort thing.

Ok.

-- 
Sabrina