Re: [PATCH 1/6] bridge: learn dst metadata in FDB

[Nikolay Aleksandrov <nikolay@cumulusnetworks.com>]

On 17/08/17 15:10, David Lamparter wrote:
> On Thu, Aug 17, 2017 at 02:51:12PM +0300, Nikolay Aleksandrov wrote:
>> On 17/08/17 14:39, Nikolay Aleksandrov wrote:
>>> On 17/08/17 14:03, David Lamparter wrote:
>>>> On Wed, Aug 16, 2017 at 11:38:06PM +0300, Nikolay Aleksandrov wrote:
> [cut]
>>>>> and hitting the fast path for everyone in a few different places for a
>>>>> feature that the majority will not use does not sound acceptable to
>>>>> me. We've been trying hard to optimize it, trying to avoid additional
>>>>> cache lines, removing tests and keeping special cases to a minimum. 
>>>>
>>>> skb->dst is on the same cacheline as skb->len.
>>>> fdb->md_dst is on the same cacheline as fdb->dst.
>>>> Both will be 0 in a lot of cases, so this should be two null checks on
>>>> data that is hot in the cache.  Are you sure this is an actual problem?
>>>>
>>>
>>> Sure - no, I haven't benchmarked it, but I don't see skb->len being on
>>> the same cache line as _skb_refdst assuming 64 byte cache lines.
>>
>> I should've been clearer - that obviously depends on the kernel config, but
>> in order for them to be in the same line you need to disable either one of 
>> conntrack, bridge_netfilter or xfrm, these are almost always enabled (at
>> least in all major distributions).
> 
> Did I miscount somewhere?  This is what I counted:
> offs    size
> 00      16      next/prev/other union bits
> 16      8       sk
> 24      8       dev
> 32	32      cb (first 32 bytes)
> ---- boundary @ 64
> 64      16      cb (last 16 bytes)
> 80      8       _skb_refdst
> 88      8       destructor
> 96      8 (0)   sp
> 104     8 (0)   _nfct
> 112     8 (0)   nf_bridge
> 120     4       len
> 124     4       data_len
> ---- boundary @ 128
> 128     2       mac_len
> 130     2       hdr_len
> 
> 
> -David
> 
What kernel ?

pahole -C sk_buff 

struct sk_buff {
	union {
		struct {
			struct sk_buff * next;           /*     0     8 */
			struct sk_buff * prev;           /*     8     8 */
			union {
				ktime_t tstamp;          /*           8 */
				u64 skb_mstamp;          /*           8 */
			};                               /*    16     8 */
		};                                       /*          24 */
		struct rb_node     rbnode;               /*          24 */
	};                                               /*     0    24 */
	struct sock *              sk;                   /*    24     8 */
	union {
		struct net_device * dev;                 /*           8 */
		long unsigned int  dev_scratch;          /*           8 */
	};                                               /*    32     8 */
	char                       cb[48];               /*    40    48 */
	/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
	long unsigned int          _skb_refdst;          /*    88     8 */
	void                       (*destructor)(struct sk_buff *); /*    96     8 */
	struct sec_path *          sp;                   /*   104     8 */
	long unsigned int          _nfct;                /*   112     8 */
	struct nf_bridge_info *    nf_bridge;            /*   120     8 */
	/* --- cacheline 2 boundary (128 bytes) --- */
	unsigned int               len;                  /*   128     4 */
	unsigned int               data_len;             /*   132     4 */