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

[Christian Hopps <chopps@chopps.org>]

[-- Attachment #1: Type: text/plain, Size: 4262 bytes --]


Steffen Klassert <steffen.klassert@secunet.com> writes:

> On Tue, Aug 06, 2024 at 04:54:53AM -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.
>
> Maybe use L1_CACHE_BYTES instead of 64? This will give you
> the actual size of the cacheline.

Yes, although a bit more than just a swap:

#define XFRM_IPTFS_MIN_L2HEADROOM (L1_CACHE_BYTES > 64 ? 64 : 64 + 16)

Here's the new comment text which explains this:

/*
 * L2 Header resv: Arrange for cacheline to start at skb->data - 16 to keep the
 * to-be-pushed L2 header in the same cacheline as resulting `skb->data` (i.e.,
 * the L3 header). If cacheline size is > 64 then skb->data + pushed L2 will all
 * be in a single cacheline if we simply reserve 64 bytes.
 */

I'm simply protecting against some new arch that decides to have 256 byte cacheline since we do not need to reserve 256 bytes for L2 headers.

>> > > > > +	skb_reserve(skb, resv);
>> > > > > +
>> > > > > +	/* We do not want any of the tpl->headers copied over, so we do
>> > > > > +	 * not use `skb_copy_header()`.
>> > > > > +	 */
>> > > >
>> > > > This is a bit of a bad sign for the implementation. It also worries
>> > > > me, as this may not be updated when changes are made to
>> > > > __copy_skb_header().
>> > > > (c/p'd from v1 review since this was still not answered)
>> > >
>> > > I don't agree that this is a bad design at all, I'm curious what you think a good design to be.
>> >
>> > Strange skb manipulations hiding in a protocol module is not good
>> > design.
>>
>> It's a fragmentation and aggregation protocol, it's needs work with skbs by design. It's literally the function of the protocol to manipulate packet content.
>>
>> I would appreciate it if you could provide technical reasons to justify referring to things as "bad" or "strange" -- it's not helpful otherwise.
>>
>> > c/p bits of core code into a module (where they will never get fixed
>> > up when the core code gets updated) is always a bad idea.
>>
>> I need some values from the SKB, so I copy them -- it's that simple.
>>
>> > > I did specifically state why we are not re-using
>> > > skb_copy_header(). The functionality is different. We are not trying
>> > > to make a copy of an skb we are using an skb as a template for new
>> > > skbs.
>> >
>> > I saw that. That doesn't mean it's a good thing to do.
>>
>> Please suggest an alternative.
>
> Maybe create a helper like this:
>
> void ___copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
> {
>         new->tstamp             = old->tstamp;
>         /* We do not copy old->sk */
>         new->dev                = old->dev;
>         memcpy(new->cb, old->cb, sizeof(old->cb));
>         skb_dst_copy(new, old);
>         __skb_ext_copy(new, old);
>         __nf_copy(new, old, false);
> }
>
> and change __copy_skb_header() to use this too. That way it gets
> updated whenever something changes here.

Ok.

Thanks,
Chris.

> It also might make sense to split out the generic infrastructure changes
> into a separate pachset wih netdev maintainers Cced on. That would make
> the changes more visible.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]