Hello!
I have a question about the value of FB_MTU in tipc, how does the '3744' form?
I notice that it is used in 'tipc_msg_build()' when memory allocation
fails, and it
tries to fall back to a smaller MTU to avoid unnecessary sending failures.
However, the size of the data allocated will be more than 4096 when FB_MTU
is 3744. I did a rough calculation, the size of data will more than 4200:
(FB_MTU + TIPCHDR + BUF_HEADROOM + sizeof(struct skb_shared_info))
Therefore, 8192 will be allocated from slab, and about 4000 of it will
not be used.
FB_MTU is used for low memory, and I think eating two pages will make it worse.
Do I miss something?
Thanks!
Menglong Dong
On 6/1/21 10:18 AM, Menglong Dong wrote:
> Hello!
>
> I have a question about the value of FB_MTU in tipc, how does the '3744' form?
> I notice that it is used in 'tipc_msg_build()' when memory allocation
> fails, and it
> tries to fall back to a smaller MTU to avoid unnecessary sending failures.
>
> However, the size of the data allocated will be more than 4096 when FB_MTU
> is 3744. I did a rough calculation, the size of data will more than 4200:
>
> (FB_MTU + TIPCHDR + BUF_HEADROOM + sizeof(struct skb_shared_info))
>
> Therefore, 8192 will be allocated from slab, and about 4000 of it will
> not be used.
>
> FB_MTU is used for low memory, and I think eating two pages will make it worse.
> Do I miss something?
>
> Thanks!
> Menglong Dong
>
Hi Dong,
The value is based on empiric knowledge.
When I determined it I made a small loop in a kernel driver where I
allocated skbs (using tipc_buf_acquire) with an increasing size
(incremented with 1 each iteration), and then printed out the
corresponding truesize.
That gave the value we are using now.
Now, when re-running the test I get a different value, so something has
obviously changed since then.
[ 1622.158586] skb(513) =>> truesize 2304, prev skb(512) => prev
truesize 1280
[ 1622.162074] skb(1537) =>> truesize 4352, prev skb(1536) => prev
truesize 2304
[ 1622.165984] skb(3585) =>> truesize 8448, prev skb(3584) => prev
truesize 4352
As you can see, the optimal value now, for an x86_64 machine compiled
with gcc, is 3584 bytes, not 3744.
Feel free to post a patch for this if you want to.
Thanks
///Jon Maloy
Hello Maloy,
On Thu, Jun 3, 2021 at 3:50 AM Jon Maloy <[email protected]> wrote:
[...]
> Hi Dong,
> The value is based on empiric knowledge.
> When I determined it I made a small loop in a kernel driver where I
> allocated skbs (using tipc_buf_acquire) with an increasing size
> (incremented with 1 each iteration), and then printed out the
> corresponding truesize.
>
> That gave the value we are using now.
>
> Now, when re-running the test I get a different value, so something has
> obviously changed since then.
>
> [ 1622.158586] skb(513) =>> truesize 2304, prev skb(512) => prev
> truesize 1280
> [ 1622.162074] skb(1537) =>> truesize 4352, prev skb(1536) => prev
> truesize 2304
> [ 1622.165984] skb(3585) =>> truesize 8448, prev skb(3584) => prev
> truesize 4352
>
> As you can see, the optimal value now, for an x86_64 machine compiled
> with gcc, is 3584 bytes, not 3744.
I'm not sure if this is a perfect way to determine the value of FB_MTU.
If 'struct skb_shared_info' changes, this value seems should change,
too.
How about we make it this:
#define FB_MTU (PAGE_SIZE - \
SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \
SKB_DATA_ALIGN(BUF_HEADROOM + BUF_TAILROOM + 3 + \
MAX_H_SIZ))
The value 'BUF_HEADROOM + BUF_TAILROOM + 3' come from 'tipc_buf_acquire()':
#ifdef CONFIG_TIPC_CRYPTO
unsigned int buf_size = (BUF_HEADROOM + size + BUF_TAILROOM + 3) & ~3u;
#else
unsigned int buf_size = (BUF_HEADROOM + size + 3) & ~3u;
#endif
Is it a good idea?
Thanks
Menglong Dong
On 6/2/21 10:26 PM, Menglong Dong wrote:
> Hello Maloy,
>
> On Thu, Jun 3, 2021 at 3:50 AM Jon Maloy <[email protected]> wrote:
>
> [...]
>> Hi Dong,
>> The value is based on empiric knowledge.
>> When I determined it I made a small loop in a kernel driver where I
>> allocated skbs (using tipc_buf_acquire) with an increasing size
>> (incremented with 1 each iteration), and then printed out the
>> corresponding truesize.
>>
>> That gave the value we are using now.
>>
>> Now, when re-running the test I get a different value, so something has
>> obviously changed since then.
>>
>> [ 1622.158586] skb(513) =>> truesize 2304, prev skb(512) => prev
>> truesize 1280
>> [ 1622.162074] skb(1537) =>> truesize 4352, prev skb(1536) => prev
>> truesize 2304
>> [ 1622.165984] skb(3585) =>> truesize 8448, prev skb(3584) => prev
>> truesize 4352
>>
>> As you can see, the optimal value now, for an x86_64 machine compiled
>> with gcc, is 3584 bytes, not 3744.
> I'm not sure if this is a perfect way to determine the value of FB_MTU.
> If 'struct skb_shared_info' changes, this value seems should change,
> too.
>
> How about we make it this:
>
> #define FB_MTU (PAGE_SIZE - \
> SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \
> SKB_DATA_ALIGN(BUF_HEADROOM + BUF_TAILROOM + 3 + \
> MAX_H_SIZ))
>
> The value 'BUF_HEADROOM + BUF_TAILROOM + 3' come from 'tipc_buf_acquire()':
>
> #ifdef CONFIG_TIPC_CRYPTO
> unsigned int buf_size = (BUF_HEADROOM + size + BUF_TAILROOM + 3) & ~3u;
> #else
> unsigned int buf_size = (BUF_HEADROOM + size + 3) & ~3u;
> #endif
>
> Is it a good idea?
Yes, I think that makes sense. I was always aware of the "fragility" of
my approach, -this one looks more future safe.
///jon
>
> Thanks
> Menglong Dong
>
On Thu, Jun 3, 2021 at 9:08 PM Jon Maloy <[email protected]> wrote:
>
[...]
> Yes, I think that makes sense. I was always aware of the "fragility" of
> my approach, -this one looks more future safe.
>
Ok, I will post a patch.
Menglong Dong