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

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

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