Messages in this thread | | | Subject | Re: [V9fs-developer] [PATCH v2] net/9p: Fix a deadlock case in the virtio transport | From | jiangyiwen <> | Date | Wed, 18 Jul 2018 08:58:38 +0800 |
| |
On 2018/7/17 21:07, Dominique Martinet wrote: > jiangyiwen wrote on Tue, Jul 17, 2018: >> On 2018/7/17 19:42, Dominique Martinet wrote: >>> >>>> Subject: net/9p: Fix a deadlock case in the virtio transport >>> >>> I hadn't noticed in the v1, but how is that a deadlock fix? >>> The previous code doesn't look like it deadlocks to me, the commit >>> message is more correct. >>> >> >> If cpu is running in the irq context for a long time, >> NMI watchdog will detect the hard lockup in the cpu, >> and then it will cause kernel panic. So I use this >> subject to underline the scenario. > > That's still not a deadlock - fix lockup would be more appropriate? > >
Okay.
>>> Do we have a guarantee that req_done is only called if there is at least >>> one buf to read? >>> For example, that there isn't two threads queueing the same callback but >>> the first one reads everything and the second has nothing to read? >>> >>> If virtblk_done takes care of setting up a "req_done" bool to only >>> notify waiters if something has been done I'd rather have a reason to do >>> differently, even if you can argue that nothing bad will happen in case >>> of a gratuitous wake_up >>> >> >> Sorry, I don't fully understand what your mean. >> I think even if the ring buffer don't have the data, wakeup operation >> will not cause any other problem, and the loss of performance can be >> ignored. > > I just mean "others do check, why not us?". It's almost free to check if > we had something to read, but if there are many pending read/writes > waiting for a buffer they will all wake up and spin uselessly. > > I've checked other callers of virtqueue_get_buf() and out of 9 that loop > around in a callback then wake another thread up, 6 do check before > waking up, two check that something happened just to print a debug > statement if not (virtio_test and virtgpu) and one doesn't check > (virtio_input); so I guess we wouldn't be the first ones, just not > following the trend. > > But yes, nothing bad will happen, so let's agree to disagree and I'll > defer to others opinion on this > > > Thanks, >
Thanks for your reply, you're right, other callers also check whether Virtio ring has data then do wakeup operation, we also should follow the trend.
Okay, I will resend the patch later.
Thanks, Yiwen.
| |