On Tue, Aug 06, 2024 at 04:54:53AM -0400, Christian Hopps wrote:

Sabrina Dubroca [off-list ref] writes:

quoted

2024-08-04, 22:33:05 -0400, Christian Hopps wrote:

quoted

quoted

quoted

+/* 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.

quoted

quoted

quoted

quoted

+
+	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.

quoted

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.

quoted

quoted

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.

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.