[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <m2o764nvgh.fsf@ja-home.int.chopps.org>
Date: Wed, 07 Aug 2024 12:23:39 -0400
From: Christian Hopps <chopps@...pps.org>
To: Steffen Klassert <steffen.klassert@...unet.com>
Cc: Christian Hopps <chopps@...pps.org>, Sabrina Dubroca
<sd@...asysnail.net>, devel@...ux-ipsec.org, netdev@...r.kernel.org,
Christian Hopps <chopps@...n.net>
Subject: Re: [PATCH ipsec-next v8 10/16] xfrm: iptfs: add fragmenting of
larger than MTU user packets
Steffen Klassert <steffen.klassert@...unet.com> writes:
> On Tue, Aug 06, 2024 at 04:54:53AM -0400, Christian Hopps wrote:
>>
>> Sabrina Dubroca <sd@...asysnail.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.
Download attachment "signature.asc" of type "application/pgp-signature" (858 bytes)
Powered by blists - more mailing lists