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, 09:35:04 -0400, Christian Hopps wrote:
> 
> 
> > On Aug 8, 2024, at 09:28, Sabrina Dubroca <sd@queasysnail.net> wrote:
> > 
> > 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?
> 
> Exactly right. When we are generating IPTFS tunnel packets we need
> to add all the L3+l2 headers, and in that case we pass l3resv =
> true.

Could you add a little comment alongside iptfs_alloc_skb? It would
help make sense of the sizes you're choosing and how they fit the use
of those skbs (something like "l3resv=true is used on TX, because we
need to reserve L2+L3 headers. On RX, we only need L2 headers because
[reason why we need L2 headers].").

And if skb->dev->needed_headroom is correct in the TX case, I'd still
prefer (skb->dev->needed_headroom + <some space for l3>) to a fixed 128.

-- 
Sabrina