Bug and patch - Heavy device io can hang

[Craig White]

I've just emailed the signed OCA to Oracle as an individual.
Craig


> On May 31, 2018, at 10:24 AM, Sergey Nazarkin <snazarkin at azul.com> wrote:
> 
> 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
>>>> 
>>>> 
>>> 
>> 
>