Bug and patch - Heavy device io can hang

[Sergey Nazarkin]

To accept the patch I need your (or your employer) OCA.  Are you individual contributor or this is part of job activity? 


Sergey Nazarkin




> On 31 May 2018, at 17:18, Craig White <cwhite102 at gmail.com> wrote:
> 
> Thanks Sergey.
> I haven't signed the contributor agreement yet.  Do you need this to accept the patch?
> I usually just use my co-worker (who has some level of contributor status) as the interface to Oracle/OpenJDK when we find issues.
> Craig
> 
> 
> -----Original Message-----
> From: Sergey Nazarkin [mailto:snazarkin at azul.com] 
> Sent: Thursday, May 31, 2018 10:07 AM
> To: Craig White
> Cc: dio-dev at openjdk.java.net
> Subject: Re: Bug and patch - Heavy device io can hang
> 
> Craig, 
> 
> The patch looks OK (at least for the moment, the whole signal/wait subsystem requires refactoring).  Have you signed Oracle Contributed Agreement (http://openjdk.java.net/contribute/)?
> 
> 
> Sergey Nazarkin
> 
> 
> 
> 
>> On 25 May 2018, at 18:32, Sergey Nazarkin <snazarkin at azul.com> wrote:
>> 
>> Hi, Craig!
>> 
>> Thanks for the fix. I’ll take a look on Monday
>> 
>> 
>> Sergey Nazarkin
>> 
>> 
>> 
>> 
>>> On 25 May 2018, at 07:57, Craig White <cwhite102 at gmail.com> wrote:
>>> 
>>> I'm not sure if this library is being maintained or not, but I found it was
>>> quite useful to get some boards going with a pi!
>>> 
>>> 
>>> 
>>> I ran into a hang situation with heavy writes to an I2C device, and traced
>>> the issue to improper mutex/condition code in dio_common.cpp.
>>> 
>>> It was very reproducible and evident that the waitForCondition method could
>>> miss the signal from generateSignal() and just get stuck there waiting.
>>> 
>>> 
>>> 
>>> Craig
>>> 
>>> 
>>> 
>>> Here's a patch to fix the issue:
>>> 
>>> 
>>> 
>>> 
>>> 
>>> @@ -279,4 +279,10 @@
>>> 
>>>       if (sig != NULL) {
>>> 
>>>           javautil_list_add(signals, sig);
>>> 
>>> +            // Need to lock the mutex associated with the condition
>>> 
>>> +            // otherwise we could miss the signal
>>> 
>>> +            // This is done while holding the 'master' signalMutex
>>> 
>>> +
>>> javacall_os_mutex_lock(javacall_os_cond_get_mutex(sig->condition));
>>> 
>>> +
>>> 
>>> +            // Now release the master lock
>>> 
>>>           javacall_os_mutex_unlock(signalMutex);
>>> 
>>> @@ -282,2 +288,4 @@
>>> 
>>>           javacall_os_mutex_unlock(signalMutex);
>>> 
>>> +
>>> 
>>> +            // Wait on the condition (releases the condition mutex while
>>> waiting)
>>> 
>>>           javacall_os_cond_wait(sig->condition, timeout);
>>> 
>>> @@ -283,4 +291,9 @@
>>> 
>>>           javacall_os_cond_wait(sig->condition, timeout);
>>> 
>>> +            // Technically, this should be in a loop for spurious wakes,
>>> but
>>> 
>>> +            // I'm not familiar enough with the code for what the loop
>>> would check
>>> 
>>> +
>>> 
>>> +            // Unlock the condition mutex
>>> 
>>> +
>>> javacall_os_mutex_unlock(javacall_os_cond_get_mutex(sig->condition));
>>> 
>>>       }
>>> 
>>>   } else {
>>> 
>>>       javacall_os_mutex_unlock(signalMutex);
>>> 
>>> @@ -313,4 +326,9 @@
>>> 
>>>   signal* sig = findTarget(signalType, signalTarget);
>>> 
>>>   if (sig != NULL) {
>>> 
>>>       sig->parameter = signalParameter;
>>> 
>>> +        
>>> 
>>> +        // Need to lock the mutex associated with the condition
>>> 
>>> +        // or waitForSignal could miss the signal
>>> 
>>> +        javacall_os_mutex_lock( javacall_os_cond_get_mutex(sig->condition)
>>> );
>>> 
>>> +
>>> 
>>>       javacall_os_cond_signal(sig->condition);
>>> 
>>> @@ -316,4 +334,6 @@
>>> 
>>>       javacall_os_cond_signal(sig->condition);
>>> 
>>> +        
>>> 
>>> +        javacall_os_mutex_unlock(
>>> javacall_os_cond_get_mutex(sig->condition) );
>>> 
>>>   } else {
>>> 
>>>       // signal is being sent before wait started
>>> 
>>>       // create the signal and add it to the list
>>> 
>>> 
>> 
>