question about size of sk_buff and skb_shared_info

question about size of sk_buff and skb_shared_info

[Code Soldier1]

Hi Folks,

I am sure there is a reason for the current sizes of these structures,
However the reason is not obvious to me. So please help me understand.

Currently the size of sk_buff on an x86_64 system is 232 bytes -- Why
is that. I expected it to be a multiple of 32/64 as they are the most
common cache lines. Since the alignment calculation will align the
structure with the hw cache line, it seems like we might be wasting
space ?

skb_shared_info on the other hand is perfectly aligned with a size of 320 bytes.

Thanks,

-- 
CS1

Re: question about size of sk_buff and skb_shared_info

[Eric Dumazet]

On Tue, 2017-04-18 at 10:34 -0700, Code Soldier1 wrote:
> Hi Folks,
> 
> I am sure there is a reason for the current sizes of these structures,
> However the reason is not obvious to me. So please help me understand.
> 
> Currently the size of sk_buff on an x86_64 system is 232 bytes -- Why
> is that. I expected it to be a multiple of 32/64 as they are the most
> common cache lines. Since the alignment calculation will align the
> structure with the hw cache line, it seems like we might be wasting
> space ?
> 
> skb_shared_info on the other hand is perfectly aligned with a size of 320 bytes.
> 
> Thanks,
> 

The alignment is there.
Look at skb_init() code, using SLAB_HWCACHE_ALIGN

Re: question about size of sk_buff and skb_shared_info

[Code Soldier1]

Eric,

This alignment flag is passed to the cache constructor and the
allocation is indeed cache aligned. However, since the allocated size
is not a multiple of the alignment, wont memory be wasted ?. We can
get 40 extra bytes without any side effects since they are on the same
cache line ?

kmem_cache_create() code does an ALIGN() to round up the size.

        kasan_cache_create(cachep, &size, &flags);

        size = ALIGN(size, cachep->align);

This is the size used in calculate_slab_order() to calculate num. I am
assuming that in the non debug case gfp_order will be 0.

Perhaps I am missing something.


On Tue, Apr 18, 2017 at 12:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2017-04-18 at 10:34 -0700, Code Soldier1 wrote:
>> Hi Folks,
>>
>> I am sure there is a reason for the current sizes of these structures,
>> However the reason is not obvious to me. So please help me understand.
>>
>> Currently the size of sk_buff on an x86_64 system is 232 bytes -- Why
>> is that. I expected it to be a multiple of 32/64 as they are the most
>> common cache lines. Since the alignment calculation will align the
>> structure with the hw cache line, it seems like we might be wasting
>> space ?
>>
>> skb_shared_info on the other hand is perfectly aligned with a size of 320 bytes.
>>
>> Thanks,
>>
>
> The alignment is there.
> Look at skb_init() code, using SLAB_HWCACHE_ALIGN
>
>
>
>
>



-- 
CS1

Re: question about size of sk_buff and skb_shared_info

[Eric Dumazet]

Please do not top post on netdev

On Tue, 2017-04-18 at 16:26 -0700, Code Soldier1 wrote:
> Eric,
> 
> This alignment flag is passed to the cache constructor and the
> allocation is indeed cache aligned. However, since the allocated size
> is not a multiple of the alignment, wont memory be wasted ?. We can
> get 40 extra bytes without any side effects since they are on the same
> cache line ?

We _want_ to align skb to cache lines.

Fact that few bytes might be wasted is unfortunate, but negligible. 

BTW, if you do kmalloc(1025), kmalloc() wastes 1023 bytes.


> 
> kmem_cache_create() code does an ALIGN() to round up the size.
> 
>         kasan_cache_create(cachep, &size, &flags);
> 
>         size = ALIGN(size, cachep->align);
> 
> This is the size used in calculate_slab_order() to calculate num. I am
> assuming that in the non debug case gfp_order will be 0.
> 
> Perhaps I am missing something.
> 
> 
> On Tue, Apr 18, 2017 at 12:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Tue, 2017-04-18 at 10:34 -0700, Code Soldier1 wrote:
> >> Hi Folks,
> >>
> >> I am sure there is a reason for the current sizes of these structures,
> >> However the reason is not obvious to me. So please help me understand.
> >>
> >> Currently the size of sk_buff on an x86_64 system is 232 bytes -- Why
> >> is that. I expected it to be a multiple of 32/64 as they are the most
> >> common cache lines. Since the alignment calculation will align the
> >> structure with the hw cache line, it seems like we might be wasting
> >> space ?
> >>
> >> skb_shared_info on the other hand is perfectly aligned with a size of 320 bytes.
> >>
> >> Thanks,
> >>
> >
> > The alignment is there.
> > Look at skb_init() code, using SLAB_HWCACHE_ALIGN
> >
> >
> >
> >
> >
> 
> 
> 

Re: question about size of sk_buff and skb_shared_info

[Code Soldier1]

I am not suggesting that we do not do cache alignment. That is
required for performance. I have gone through this exercise because I
need to add a field to sk_buff and I want to do that without causing
any adverse effects.

Now that we have discovered that there are 40 bytes that can be used
without any adverse effect, may I increase skb->cb by 8 bytes ?

If not then may I increase skb_shared_info -- However that would have
to be by 64bytes.


On Tue, Apr 18, 2017 at 4:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Please do not top post on netdev
>
> On Tue, 2017-04-18 at 16:26 -0700, Code Soldier1 wrote:
>> Eric,
>>
>> This alignment flag is passed to the cache constructor and the
>> allocation is indeed cache aligned. However, since the allocated size
>> is not a multiple of the alignment, wont memory be wasted ?. We can
>> get 40 extra bytes without any side effects since they are on the same
>> cache line ?
>
> We _want_ to align skb to cache lines.
>
> Fact that few bytes might be wasted is unfortunate, but negligible.
>
> BTW, if you do kmalloc(1025), kmalloc() wastes 1023 bytes.
>
>
>>
>> kmem_cache_create() code does an ALIGN() to round up the size.
>>
>>         kasan_cache_create(cachep, &size, &flags);
>>
>>         size = ALIGN(size, cachep->align);
>>
>> This is the size used in calculate_slab_order() to calculate num. I am
>> assuming that in the non debug case gfp_order will be 0.
>>
>> Perhaps I am missing something.
>>
>>
>> On Tue, Apr 18, 2017 at 12:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > On Tue, 2017-04-18 at 10:34 -0700, Code Soldier1 wrote:
>> >> Hi Folks,
>> >>
>> >> I am sure there is a reason for the current sizes of these structures,
>> >> However the reason is not obvious to me. So please help me understand.
>> >>
>> >> Currently the size of sk_buff on an x86_64 system is 232 bytes -- Why
>> >> is that. I expected it to be a multiple of 32/64 as they are the most
>> >> common cache lines. Since the alignment calculation will align the
>> >> structure with the hw cache line, it seems like we might be wasting
>> >> space ?
>> >>
>> >> skb_shared_info on the other hand is perfectly aligned with a size of 320 bytes.
>> >>
>> >> Thanks,
>> >>
>> >
>> > The alignment is there.
>> > Look at skb_init() code, using SLAB_HWCACHE_ALIGN
>> >
>> >
>> >
>> >
>> >
>>
>>
>>
>
>



-- 
CS1

Re: question about size of sk_buff and skb_shared_info

[Eric Dumazet]

For the last time, do not top post on netdev.

On Tue, 2017-04-18 at 17:15 -0700, Code Soldier1 wrote:
> I am not suggesting that we do not do cache alignment. That is
> required for performance. I have gone through this exercise because I
> need to add a field to sk_buff and I want to do that without causing
> any adverse effects.
> 
> Now that we have discovered that there are 40 bytes that can be used
> without any adverse effect, may I increase skb->cb by 8 bytes ?
> 

skb->cb is already 48 bytes, not 40.

> If not then may I increase skb_shared_info -- However that would have
> to be by 64bytes.


You will have a very hard time to convince us that this 8 byte field is
needed on all skbs, regardless of current sk_buff size.



> 
> 
> On Tue, Apr 18, 2017 at 4:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Please do not top post on netdev
> >
> > On Tue, 2017-04-18 at 16:26 -0700, Code Soldier1 wrote:
> >> Eric,
> >>
> >> This alignment flag is passed to the cache constructor and the
> >> allocation is indeed cache aligned. However, since the allocated size
> >> is not a multiple of the alignment, wont memory be wasted ?. We can
> >> get 40 extra bytes without any side effects since they are on the same
> >> cache line ?
> >
> > We _want_ to align skb to cache lines.
> >
> > Fact that few bytes might be wasted is unfortunate, but negligible.
> >
> > BTW, if you do kmalloc(1025), kmalloc() wastes 1023 bytes.
> >
> >
> >>
> >> kmem_cache_create() code does an ALIGN() to round up the size.
> >>
> >>         kasan_cache_create(cachep, &size, &flags);
> >>
> >>         size = ALIGN(size, cachep->align);
> >>
> >> This is the size used in calculate_slab_order() to calculate num. I am
> >> assuming that in the non debug case gfp_order will be 0.
> >>
> >> Perhaps I am missing something.
> >>
> >>
> >> On Tue, Apr 18, 2017 at 12:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> > On Tue, 2017-04-18 at 10:34 -0700, Code Soldier1 wrote:
> >> >> Hi Folks,
> >> >>
> >> >> I am sure there is a reason for the current sizes of these structures,
> >> >> However the reason is not obvious to me. So please help me understand.
> >> >>
> >> >> Currently the size of sk_buff on an x86_64 system is 232 bytes -- Why
> >> >> is that. I expected it to be a multiple of 32/64 as they are the most
> >> >> common cache lines. Since the alignment calculation will align the
> >> >> structure with the hw cache line, it seems like we might be wasting
> >> >> space ?
> >> >>
> >> >> skb_shared_info on the other hand is perfectly aligned with a size of 320 bytes.
> >> >>
> >> >> Thanks,
> >> >>
> >> >
> >> > The alignment is there.
> >> > Look at skb_init() code, using SLAB_HWCACHE_ALIGN
> >> >
> >> >
> >> >
> >> >
> >> >
> >>
> >>
> >>
> >
> >
> 
> 
> 

Re: question about size of sk_buff and skb_shared_info

[Code Soldier1]

On Tue, Apr 18, 2017 at 5:47 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> For the last time, do not top post on netdev.

Sorry about that.
>
> On Tue, 2017-04-18 at 17:15 -0700, Code Soldier1 wrote:
>> I am not suggesting that we do not do cache alignment. That is
>> required for performance. I have gone through this exercise because I
>> need to add a field to sk_buff and I want to do that without causing
>> any adverse effects.
>>
>> Now that we have discovered that there are 40 bytes that can be used
>> without any adverse effect, may I increase skb->cb by 8 bytes ?
>>
>
> skb->cb is already 48 bytes, not 40.

That is not what I am referring to above. What I am pointing out is
that since skb is not a multiple of cache line size it accesses the
whole cache line but uses only part of the cache line, leaving 40
bytes unused. I would like to use 8 out of those 40 bytes to extend
the size of skb->cb.

>
>> If not then may I increase skb_shared_info -- However that would have
>> to be by 64bytes.
>
>
> You will have a very hard time to convince us that this 8 byte field is
> needed on all skbs, regardless of current sk_buff size.
>

Protocols use as much space as needed and leave the rest for future
use. Since there is no overhead, I don't understand the concern. Can
you please elaborate.

-- 
CS1