On 09/28/2017 06:00 AM, Erik Skultety wrote:
> [...]
>
>>>
>>> nodeDeviceLock();
>>> + priv = driver->privateData;
>>> udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
>>>
>>> if (!udevEventCheckMonitorFD(udev_monitor, privateData->monitor_fd)) {
>>> @@ -1725,6 +1727,9 @@ udevEventHandleThread(void *opaque)
>>> device = udev_monitor_receive_device(udev_monitor);
>>> nodeDeviceUnlock();
>>>
>>> + /* Re-enable polling for new events on the @udev_monitor */
>>> + virEventUpdateHandle(priv->watch, VIR_EVENT_HANDLE_READABLE);
>>> +
>>
>> I think this should only be done when privateData->nevents == 0? If we
>> have multiple events to read, then calling virEventPollUpdateHandle,
>> (eventually) for every pass through the loop seems like a bit of
>> overkill especially if udevEventHandleCallback turns right around and
>> disables it again.
>>
>> Also fortunately there isn't more than one udev thread sending the
>> events since you access the priv->watch without the driver lock...
>>
>> Conversely perhaps we only disable if events > 1... Then again, how does
>> one get to 2 events queued if we're disabling as soon as we increment
>
> Very good point, technically events would still get queued, we just wouldn't
> check and yes, we would process 1 event at a time. Not optimal, but if you look
> at the original code and compare it with this one performance-wise (and I hope
> I haven't missed anything that would render everything I write next a complete
> rubbish), the time complexity hasn't changed, the space complexity hasn't
> changed, what changed is code complexity which makes the code a bit slower due
> to the excessive locking and toggling the FD polling back and forth. So
> essentially you've got the same thing as you had before..but asynchronous.
> However, yes, the usage of @nevents is completely useless now (haven't realized
> that immediately, thanks) and a simple signalling should suffice.
Having it "slower" is necessarily bad ;-) That gives some of the other
slower buggers a chance to fill in the details we need. Throwing the
control back to udev quicker could aid in that too.
>
> So how could we make it faster though? I thought more about the idea you shared
> in one of the previous reviews, letting the thread actually pull all the data
> from the monitor, to which I IIRC replied something in the sense that the event
> counting mechanism wouldn't allow that and it would break. Okay, let's drop the
> event counting. What if we now let both the udev handler thread and the event
> loop "poll" the file descriptor, IOW let the event loop polling the monitor fd,
> thus invoking udevHandleCallback which would in turn keep signalling the handler
> thread that there are some data. The difference now in the handler thread would
> be that it wouldn't blindly trust the callback about the data, because of the
> scheduling issue, it would keep poking the monitor itself until it gets either
> EAGAIN or EWOULDBLOCK from recvmsg() called in libudev, which would then be the
> signal to start waiting on the condition. The next an event appears, a signal
> from udevHandleCallback would finally have a meaning and wouldn't be ignored.
Is making it faster really a goal? It's preferable that it works
consistently I think. The various errno possibilities and the desire to
avoid the "udev_monitor_receive_device returned NULL" message processing
because we had too many "cooks in the kitchen" trying to determine
whether a new device was really available or was this just another
notification for something we're already processing.
Also, not that I expect the udev code to change, but if a new errno is
added then we may have to keep up... Always a fear especially if we're
using the errno to dictate our algorithm.
>
> This way, it's actually the handler thread who's 'the boss', as most of the
> signals from udevHandleCallback would be lost/NOPs.
>
> Would you agree to such an approach?
At some point we get too fancy and think too hard about a problem
letting it consume us. I think when I presented my idea - I wasn't
really understanding the details of how we consume the udev data. Now
that I have a bit more of a clue - perhaps the original idea wasn't so
good. If we receive multiple notifications that a device is ready to be
processed even though we could be processing it - how are we to truly
know whether we missed one or we really got it and udev was just
reminding us again.
I'm not against looking at a different mechanism - the question then
becomes from your perspective is it worth it?
John
>
> Erik
>
>> nevents? Wouldn't this totally obviate the need for nevents?
>>
>> I would think it would be overkill to disable/enable for just 1 event.
>> If multiple are queued, then yeah we're starting to get behind... Of
>> course that could effect the re-enable when events == 1 (or some
>> MAGIC_CONSTANT_NUMBER_THRESHOLD_TO_DISABLE_POLLING)
>
> This magic threshold approach still wouldn't work because you don't know
> anything about the event at that point, you'd have to process them to actually
> find out whether that's a legitimate event or it's an already noted event that
> hasn't been processed yet, so it would still mess with your counters.
>
>>
>> IIRC from the previous trip down this rabbit hole (I did briefly go back
>> and read the comments) that what you're trying to avoid is the following
>> message spamming the error logs because of a scheduling mishap... Thus
>> perhaps the following message could only be displayed if nevents > 1 and
>> nothing was found knowing that we have this "timing problem" between
>> udev, this thread, and the fd polling scanner of when a device should be
>> "found"... and then reset nevents == 0 with the appropriate lock.
>>
>> Curious on your thoughts before an R-B/ACK though.
>>
>> John
>>
>>
>>> if (!device) {
>>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> _("udev_monitor_receive_device returned NULL"));
>>> @@ -1750,10 +1755,12 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>>> int events ATTRIBUTE_UNUSED,
>>> void *opaque)
>>> {
>>> + udevPrivate *priv = NULL;
>>> struct udev_monitor *udev_monitor = NULL;
>>> udevEventThreadDataPtr threadData = opaque;
>>>
>>> nodeDeviceLock();
>>> + priv = driver->privateData;
>>> udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
>>>
>>> if (!udevEventCheckMonitorFD(udev_monitor, fd)) {
>>> @@ -1771,6 +1778,16 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>>> threadData->nevents++;
>>> virCondSignal(&threadData->threadCond);
>>> virMutexUnlock(&threadData->lock);
>>> +
>>> + /* Due to scheduling, the eventloop might poll the udev monitor before the
>>> + * handler thread actually removes the data from the socket, thus causing it
>>> + * to spam errors about data not being ready yet, because
>>> + * udevEventHandleCallback would be invoked multiple times incrementing the
>>> + * counter of events queuing for a single event.
>>> + * Therefore we need to disable polling on the fd until the thread actually
>>> + * removes the data from the socket.
>>> + */
>>> + virEventUpdateHandle(priv->watch, 0);
>>> }
>>>
>>>
>>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
