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

[Jesper Wilhelmsson]

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
>