netdev - Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5260D9B3.2090301@oracle.com>
Date:	Fri, 18 Oct 2013 14:48:19 +0800
From:	annie li <annie.li@...cle.com>
To:	jianhai luan <jianhai.luan@...cle.com>
CC:	David Vrabel <david.vrabel@...rix.com>,
	Wei Liu <wei.liu2@...rix.com>, ian.campbell@...rix.com,
	netdev@...r.kernel.org, Jan Beulich <JBeulich@...e.com>,
	xen-devel@...ts.xenproject.org
Subject: Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now
 beyond the range time_after_eq().


On 2013-10-17 23:41, jianhai luan wrote:
>
> On 2013-10-17 23:25, David Vrabel wrote:
>> On 17/10/13 16:23, jianhai luan wrote:
>>> On 2013-10-17 22:06, Wei Liu wrote:
>>>> On Thu, Oct 17, 2013 at 09:59:30PM +0800, jianhai luan wrote:
>>>> [...]
>>>>>>>>> If use time_after_eq64(), expire ,next_credit and other member
>>>>>>>>> will must
>>>>>>>>> be u64.
>>>>>>>> Yes, you'll need to store next_credit as a u64 in vif instead of
>>>>>>>> calculating it in tx_credit_exceeded from expires (which is 
>>>>>>>> only an
>>>>>>>> unsigned long).
>>>>>>> I know that.  Even we use u64, time_after_eq()  will also do wrong
>>>>>>> judge
>>>>>>> in theory (not in reality because need long long time).
>>>>>> If jiffies_64 has millisecond resolution that would be more than
>>>>>> 500,000,000 years.
>>>>> Yes, I agree the fact.
>>>>>>> I think the two better fixed way is below:
>>>>>>>     - By time_before() to judge if now beyond MAX_ULONG/2
>>>>>> This is broken, so no.
>>>>> Where is broken?  would you like to help me point it out.
>>>> I think David means you didn't actually fix the problem. Your 
>>>> solution is
>>>> merely a workaround.
>>> I have think  about using u64, but more code need to be modified and
>>> that is not all.  Key point is how to change the element of struct
>>> time_list (expires)  and don't affect other thing?
>> I already suggested a way that didn't require changing the timer
>> structure -- calculate and store next_credit in advanced.
> I think that  modify next_credit only  will not fix the issue. please 
> think about:
>   - If jiffies have beyond 32 bit. i assume expire  is 0, jiffies_64 
> is  0x1000000ff.
>     next_credit = 0 +  <always 32-bit value >
>
>     time_after_eq64(jiffies_64, next_credit ) will always true. 
> replenish will always do, rate control will lost their function.

At first, the case above only exists when the network device keep idle 
for a long time, not frequently happens. If this case really happens, it 
means lots of jiffies are available for the credit, so there is no 
necessary to add the timer. The code operates correctly and rate control 
does not lose. This case can be shown with following config,
------old_next_credit(expires replaced)----------next_credit--------now----

So till now, two solutions are available: one is the current one to 
change if condition, another is to change all connected variant to 64. I 
incline to the former one since it involves less code change than the 
latter one.

Thanks
Annie

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html