Re: [tipc-discussion] Strange behavior in socket.c::tipc_sk_enqueue()

[Jon M. <jm...@re...>]

On 08/09/2021 19:10, Hoang Huu Le wrote:
> 
> 
>> -----Original Message-----
>> From: Jon Maloy <jm...@re...>
>> Sent: Thursday, September 9, 2021 5:42 AM
>> To: Hoang Huu Le <hoa...@de...>; tip...@li...; Tung Quang Nguyen
>> <tun...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
>> Cc: Huy Xuan Nhat Hoang <huy...@de...>
>> Subject: Re: Strange behavior in socket.c::tipc_sk_enqueue()
>>
>>
>>
>> On 06/09/2021 05:02, Hoang Huu Le wrote:
>>> Hi Jon,  all,
>>>
>>> I did a test by setting two variables condition in range:
>>> - time limit:  2 msecs ... unlimited
>>> - search depth limit (sock's skbs): 2 skbs ... unlimited
>>>
>>> With above range settings, a maximum sock's skbs can be enqueued around 12 skbs regardless of time and search depth limit.
>>> I also combine the test with iperf TCP traffic generated and the result looks the same.
>>>
>>> So, I don't think we need to apply the search depth limit condition and/or longer timer in this function, just 2msecs is enough.
>>> I guess this result depends on kernel schedule. What are your views?
>>
>> I assume your test was done with many, e.g. 100 connections?
> 
> Yes, I did the test from 1 to 150 connections and combine with/out other traffic generate (i.e TCP).

Ok. The simpler the better. So, I suggest you post it so we can have a look.

///jon

> 
>>
>> ///jon
>>
>>>
>>> Regards,
>>> Hoang
>>>> -----Original Message-----
>>>> From: Jon Maloy <jm...@re...>
>>>> Sent: Wednesday, September 1, 2021 7:39 AM
>>>> To: Hoang Huu Le <hoa...@de...>; tip...@li...; Tung Quang Nguyen
>>>> <tun...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
>>>> Cc: Huy Xuan Nhat Hoang <huy...@de...>
>>>> Subject: Re: Strange behavior in socket.c::tipc_sk_enqueue()
>>>>
>>>> Guys,
>>>> After our discussion this morning regarding this problem I gave it some
>>>> more thought.
>>>>
>>>> What if we simply limit the search depth in the receive queue to some
>>>> fix number, 10, 20, 50 or something and return NULL if nothing is found
>>>> within this range. This would be a simple stack counter inside
>>>> tipc_skb_dequeue(), and would cost almost nothing.
>>>>
>>>> If you experiment with this, of course in combination with a max limit
>>>> of some milliseconds as we also discussed, we might obtain acceptable
>>>> results.
>>>>
>>>> What do you think?
>>>>
>>>> ///jon
>>>>
>>>>
>>>> On 28/07/2021 04:04, Hoang Huu Le wrote:
>>>>> Hi Jon,
>>>>>
>>>>> Let's enjoy your vacation.
>>>>> Our new team member (CCed) will take a look at it.
>>>>>
>>>>> Regards,
>>>>> Hoang
>>>>>> -----Original Message-----
>>>>>> From: Jon Maloy <jm...@re...>
>>>>>> Sent: Wednesday, July 28, 2021 6:20 AM
>>>>>> To: tip...@li...; Tung Quang Nguyen <tun...@de...>; Hoang Huu Le
>>>>>> <hoa...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
>>>>>> Subject: Strange behavior in socket.c::tipc_sk_enqueue()
>>>>>>
>>>>>> I did by accident discover a strange behavior in the function
>>>>>> tipc_sk_enqueue:
>>>>>>
>>>>>>
>>>>>> static void tipc_sk_enqueue(struct sk_buff_head *inputq,
>>>>>>                                 struct sock *sk, u32 dport,
>>>>>>                                 struct sk_buff_head *xmitq)
>>>>>> {
>>>>>>             struct tipc_sock *tsk = tipc_sk(sk);
>>>>>>             unsigned long time_limit = jiffies + 2;
>>>>>>             struct sk_buff *skb;
>>>>>>             unsigned int lim;
>>>>>>             atomic_t *dcnt;
>>>>>>             u32 onode;
>>>>>>
>>>>>>             while (skb_queue_len(inputq)) {
>>>>>>             if (unlikely(time_after_eq(jiffies, time_limit)))
>>>>>>                   return;
>>>>>>             [...]
>>>>>>             }
>>>>>> }
>>>>>>
>>>>>> At the moment we call time_after_eq() the two jiffies often
>>>>>> have already passed, and the skb is not dequeued.
>>>>>> I noticed that tipc_sk_rcv() may call tipc_sk_enqueue()
>>>>>> with the same skb dozens of times before the buffer can
>>>>>> be delivered further upwards in the stack.
>>>>>>
>>>>>> Needless to say that this cannot be good for performance.
>>>>>>
>>>>>> I believe the value of 2 jiffies was hard coded at a time
>>>>>> when machines were slower, and a jiffie represented a much
>>>>>> longer time interval.
>>>>>>
>>>>>> Now it is clearly too short, and should be replaced with something
>>>>>> longer and more consisten, e.g. msec_to_jiffies(2).
>>>>>>
>>>>>> Can anybody look into this?
>>>>>>
>>>>>> Also, I will be on vacation the next two weeks, which means we
>>>>>> should cancel the bi-weekly meeting next Tuesday.
>>>>>>
>>>>>> ///jon
>>>>>>
>>>>>
>>>
>