On 2019-01-03 14:33, Thomas Huth wrote:
> On 2019-01-03 14:23, Janosch Frank wrote:
>> On 03.01.19 14:15, Thomas Huth wrote:
>>> On 2019-01-03 14:13, Janosch Frank wrote:
>>>> On 03.01.19 13:58, Thomas Huth wrote:
>>>>> On 2019-01-03 11:08, Janosch Frank wrote:
>>>>>> We need to properly implement interrupt handling for SCLP, because on
>>>>>> z/VM and LPAR SCLP calls are not synchronous!
>>>>>>
>>>>>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>>>> [...]
>>>>>> +
>>>>>> static void sclp_read_scp_info(ReadInfo *ri, int length)
>>>>>> {
>>>>>> unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
>>>>>> SCLP_CMDW_READ_SCP_INFO };
>>>>>> - int i;
>>>>>> + int i, cc;
>>>>>>
>>>>>> for (i = 0; i < ARRAY_SIZE(commands); i++) {
>>>>>> memset(&ri->h, 0, sizeof(ri->h));
>>>>>> ri->h.length = length;
>>>>>>
>>>>>> - if (sclp_service_call(commands[i], ri))
>>>>>> + sclp_mark_busy();
>>>>>> + cc = sclp_service_call(commands[i], ri);
>>>>>> + sclp_wait_busy();
>>>>>
>>>>> You already do the sclp_wait_busy() in sclp_service_call now, so I think
>>>>> you don't need the sclp_wait_busy() here anymore?
>>>>
>>>> Yeah, that has to go.
>>>>
>>>>>
>>>>> Also, what about moving the sclp_mark_busy() calls to the beginning of
>>>>> sclp_service_call() instead?
>>>>
>>>> Wouldn't that create a race on the data of __sccb and we could end with
>>>> garbled scb commands?
>>>
>>> Since there is a sclp_wait_busy in sclp_service_call already, you can be
>>> sure that the previous command already finished, can't you?
>>
>> I mean before calling sclp_service_call.
>> That's only a problem for smp, but before calling sclp, we prepare the
>> data in the shared __sccb page and that is currently protected by the
>> busy mark. If it's not, then two threads could write to __sccb at the
>> same time and the first that succeeds to call sclp will get a mix of
>> data of both threads.
>
> But in that case, you also need to do a sclp_wait_busy() before calling
> sclp_mark_busy() in all locations - otherwise the code is also not
> thread-safe in its current shape, is it?
Ah, never mind, now I had a look at patch 11/13, and now it makes sense.
So maybe fold patch 11 into this one, to make this clear right from the
start?
Thomas
Attachment:
signature.asc
Description: OpenPGP digital signature
