Re: [PATCH 3/3] xen: optimize xenbus driver for multiple concurrent xenstore accesses

From: Boris Ostrovsky
Date: Tue Jan 10 2017 - 11:36:12 EST



>>> +static int process_msg(void)
>>> +{
>>> + static struct xs_thread_state_read state;
>>> + struct xb_req_data *req;
>>> + int err;
>>> + unsigned int len;
>>> +
>>> + if (!state.in_msg) {
>>> + state.in_msg = true;
>>> + state.in_hdr = true;
>>> + state.used = 0;
>>> +
>>> + /*
>>> + * We must disallow save/restore while reading a message.
>>> + * A partial read across s/r leaves us out of sync with
>>> + * xenstored.
>>> + */
>>> + mutex_lock(&xs_response_mutex);
>>> +
>>> + if (!xb_data_to_read()) {
>>> + /* We raced with save/restore: pending data 'gone'. */
>>> + mutex_unlock(&xs_response_mutex);
>>> + state.in_msg = false;

Just noticed: should in_hdr be set to false here as well?

>>> + return 0;
>>> + }

Or set it to true here.

>>> + }
>>> +
>>> + if (state.in_hdr) {
>>> + if (state.used != sizeof(state.msg)) {
>>> + err = xb_read((void *)&state.msg + state.used,
>>> + sizeof(state.msg) - state.used);
>>> + if (err < 0)
>>> + goto out;
>>> + state.used += err;
>>> + if (state.used != sizeof(state.msg))
>>> + return 0;
>> Would it be possible to do locking at the caller? I understand that you
>> are trying to hold the lock across multiple invocations of this function
>> but it feels somewhat counter-intuitive and bug-prone.
> I think that would be difficult.
>
>> If it's not possible then at least please add a comment explaining
>> locking algorithm.
> Okay. Something like:
>
> /*
> * xs_response_mutex is locked as long as we are processing one
> * message. state.in_msg will be true as long as we are holding the
> * lock in process_msg().


Then in_msg is the same as mutex_is_locked(&xs_response_mutex). And if
so, do we really need it?


-boris