Michael Kelley [off-list ref] writes:

From: Andrea Parri <parri.andrea@gmail.com> Sent: Saturday, March 28, 2020 10:09 AM

quoted

quoted

In case we believe that OFFER -> RESCINF sequence is always ordered
by the host AND we don't care about other offers in the queue the
suggested locking is OK: we're guaranteed to process RESCIND after we
finished processing OFFER for the same channel. However, waiting for
'offer_in_progress == 0' looks fishy so I'd suggest we at least add a
comment explaining that the wait is only needed to serialize us with
possible OFFER for the same channel - and nothing else. I'd personally
still slightly prefer the algorythm I suggested as it guarantees we take
channel_mutex with offer_in_progress == 0 -- even if there are no issues
we can think of today (not strongly though).

Does it?  offer_in_progress is incremented without channel_mutex...

No, it does not, you're right, by itself the change is insufficient.

quoted

IAC, I have no objections to apply the changes you suggested.  To avoid
misunderstandings: vmbus_bus_suspend() presents a similar usage...  Are
you suggesting that I apply similar changes there?

Alternatively:  FWIW, the comment in vmbus_onoffer_rescind() does refer
to "The offer msg and the corresponding rescind msg...".  I am all ears
if you have any concrete suggestions to improve these comments.

Given that waiting for 'offer_in_progress == 0' is the current code, I think
there's an argument to made for not changing it if the change isn't strictly
necessary.  This patch set introduces enough change that *is* necessary. :-)

Sure. I was thinking a bit more about this and it seems that over years
we've made the synchronization of channels code too complex (every time
for a good reason but still). Now (before this series) we have at least:

vmbus_connection.channel_mutex
vmbus_connection.offer_in_progress
channel.probe_done
channel.rescind
Workqueues (vmbus_connection.work_queue,
 queue_work_on(vmbus_connection.connect_cpu),...)
channel.lock spinlock (the least of the problems)

Maybe there's room for improvement? Out of top of my head I'd suggest a
state machine for each channel (e.g something like
OFFERED->OPENING->OPEN->RESCIND_REQ->RESCINDED->CLOSED) + refcounting
(subchannels, open/rescind/... requests in progress, ...) + non-blocking
request handling like "Can we handle this rescind offer now? No,
refcount is too big. OK, rescheduling the work". Maybe not the best
design ever and I'd gladly support any other which improves the
readability of the code and makes all state changes and synchronization
between them more obvious.

Note, VMBus channel handling driven my messages (unlike events for ring
buffer) is not performance critical, we just need to ensure completeness
(all requests are handled correctly) with forward progress guarantees
(no deadlocks).

I understand the absence of 'hot' issues in the current code is what can
make the virtue of redesign questionable and sorry for hijacking the
series which doesn't seem to make things worse :-)

-- 
Vitaly