RFR (S): 8008314 - Unimplemented() Atomic::load breaks the applications

[Bengt Rutisson]

On 2/19/13 12:39 PM, David Holmes wrote:
> I'm also indebted to Bengt for spotting that - hadn't realized the 'x' 
> types were intptr_t typedefs.
>
> But backing up a step, this code is not obviously thread-safes, or if 
> it is it must be by a lock at a higher level - so why do we need an 
> atomic update of the _bytes_written value?

Yes, this is something we discussed at lunch today. It looks like 
calling update_position() without some kind of locking is unsafe. This 
is not introduced only by the rotatingFileStream. All the outputStream 
implmentations does this. Looks scary to me. I don't really understand 
how this works in a safe way. Maybe it doesn't.

We are not doing any locking higher up in the call chain as far as I can 
see.

Bengt

>
> David
>
> On 19/02/2013 8:18 PM, Jesper Wilhelmsson wrote:
>> Bengt,
>>
>> Thanks for catching this! I should listen to Ted's mother, nothing good
>> happens after 2am[1].
>>
>> I wasn't aware that jint was 32-bit on all platforms. Using 32-bit
>> atomic add like this won't work, back to the drawing board.
>>
>> The one quick solution I can think of right now that won't be awful
>> would be to limit GcLogFileSize to 32 bit. I'm not sure how quick a fix
>> it will be though since it may require a CCC approval...
>>
>> /Jesper
>>
>>
>> [1] http://en.wikipedia.org/wiki/Nothing_Good_Happens_After_2_A.M.
>>
>>
>> On 19/2/13 9:08 AM, Bengt Rutisson wrote:
>>>
>>> Hi Jesper,
>>>
>>> Thanks for fixing this! I don't feel very comfortable with this code,
>>> so don't
>>> consider this a full review. Just a couple of comments:
>>>
>>> It seems to make sense to use uintx for _bytes_written since the flag
>>> to control
>>> the file size, GCLogFileSize, is uintx. But since the atomics you 
>>> use are
>>> handling jint I think it looks a bit suspicious. jint is always 32
>>> bits and
>>> uintx can be 32 or 64 bit. So, what happens here if _bytes_written is
>>> 64 bit?
>>>
>>>   457     Atomic::add((jint)count, (jint*)&_bytes_written);
>>>
>>> If _bytes_written is 0x00000000FFFFFFFF and count is 1, I would guess
>>> that we
>>> get 0x0 instead of 0x0000000100000000.
>>>
>>> Also, in rotatingFileStream::rotate_log() you changed to:
>>>
>>> if (_bytes_written <= GCLogFileSize) return;
>>>
>>> instead of :
>>>
>>> if (_bytes_writen < (jlong)GCLogFileSize) return;
>>>
>>> isn't this a problem since you set _bytes_written to UINT_MAX if we
>>> wrap around?
>>> If someone sets GCLogFileSize on the command line to be UINT_MAX we
>>> will never
>>> rotate the file, right?
>>>
>>> Also, we only call rotate_log() when we start a safepoint. If we wrap
>>> around
>>> once in write() but then call it again before a safepoint happens we 
>>> can
>>> probably set _bytes_written to a value that looks small enough to not
>>> have to
>>> rotate the log. It seems to me that we would need a separate flag to
>>> say that we
>>> written enough to rotate the log.
>>>
>>> Bengt
>>>
>>>
>>>
>>> On 2/19/13 7:25 AM, Jesper Wilhelmsson wrote:
>>>> Hi,
>>>>
>>>> Please review the following change:
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8008314/
>>>>
>>>> Bug: https://jbs.oracle.com/bugs/browse/JDK-8008314
>>>>
>>>>
>>>> The use of atomic::add(jlong,jlong*) breaks everything on ARM since
>>>> it uses
>>>> atomic::load(jlong) which isn't implemented.
>>>>
>>>> The add was used to update a counter in rotating log file handling,
>>>> but it
>>>> turns out that the counter that was updated doesn't need to be a 
>>>> jlong.
>>>>
>>>> The counter is used to indicate that it is time to do a log file
>>>> rotation and
>>>> by adding a saftey catch for overflow, we can use an uintx for the
>>>> counter.
>>>>
>>>> In practice this means that the upper "limit" for the log file size
>>>> becomes
>>>> UINX_MAX-1 (used to be UINT_MAX) since I change from < to <= in the
>>>> comparison
>>>> to the limit, but since this is not a hard limit the difference will
>>>> only be
>>>> noticeable when someone has written UINT_MAX characters to the log
>>>> file and
>>>> the next write will end up in the next log file part instead of in
>>>> the same
>>>> (which one could argue is a more correct behavior).
>>>>
>>>> Sine I changed all places where the counter was used I also fixed a
>>>> typo in
>>>> its name.
>>>>
>>>> Thanks,
>>>> /Jesper
>>>