Messages in this thread | | | From | KY Srinivasan <> | Subject | RE: [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in interrupt context | Date | Thu, 10 Dec 2015 23:23:06 +0000 |
| |
> -----Original Message----- > From: Dexuan Cui > Sent: Thursday, December 10, 2015 2:19 AM > To: KY Srinivasan <kys@microsoft.com>; gregkh@linuxfoundation.org; linux- > kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de; > apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com > Subject: RE: [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in > interrupt context > > > -----Original Message----- > > From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On > Behalf > > Of K. Y. Srinivasan > > Sent: Friday, October 30, 2015 9:13 > > To: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; > > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com; > > vkuznets@redhat.com; jasowang@redhat.com > > Subject: [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in > > interrupt context > > > > From: Olaf Hering <olaf@aepfle.de> > > > > All channel interrupts are bound to specific VCPUs in the guest > > at the point channel is created. While currently, we invoke the > > polling function on the correct CPU (the CPU to which the channel > > is bound to) in some cases we may run the polling function in > > a non-interrupt context. This potentially can cause an issue as the > > polling function can be interrupted by the channel callback function. > > Fix the issue by running the polling function on the appropriate CPU > > at interrupt level. Additional details of the issue being addressed by > > this patch are given below: > > > > Currently hv_fcopy_onchannelcallback is called from interrupts and also > > via the ->write function of hv_utils. Since the used global variables to > > maintain state are not thread safe the state can get out of sync. > > This affects the variable state as well as the channel inbound buffer. > > > > As suggested by KY adjust hv_poll_channel to always run the given > > callback on the cpu which the channel is bound to. This avoids the need > > for locking because all the util services are single threaded and only > > one transaction is active at any given point in time. > > > > Additionally, remove the context variable, they will always be the same as > > recv_channel. > > > > Signed-off-by: Olaf Hering <olaf@aepfle.de> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> > > --- > > V2: Added the check to catch unsolicited daemon writes - Vitaly > > > > drivers/hv/hv_fcopy.c | 34 +++++++++++++--------------------- > > drivers/hv/hv_kvp.c | 28 ++++++++++------------------ > > drivers/hv/hv_snapshot.c | 29 +++++++++++------------------ > > drivers/hv/hyperv_vmbus.h | 6 +----- > > 4 files changed, 35 insertions(+), 62 deletions(-) > > (Sorry for not joining the discussion when the patch was firstly made) > > It looks the patch has not been Greg's tree yet. > > I have 2 questions about the patch: > > 1. hv_poll_channel() is invoked in fcopy_handle_handshake(), but not in > vss_handle_handshake() and kvp_handle_handshake(). > Why -- I guess we missed the vss/kvp cases somehow? I will fix this.
> > 2. With the patch, hv_fcopy_onchannelcallback() can be invoked in the > tasklet (i.e., vmbus_on_event(). NB: local irq is enabled), and in the > hard irq handler(the IPI handler, e.g., > fcopy_poll_wrapper() -> fcopy_poll_wrapper()). > > Can the former be interrupted by the latter? > e.g., when the callback is running in the tasklet on vCPU0, > fcopy_timeout_func() or fcopy_on_msg() could send the IPI to > vCPU0 from another vCPU.
Keep in mind that when the poll function is run, the state will not be HVUTIL_READY. The state will be set to HVUTIL_READY in the IPI handler. So, it is ok if the tasklet is interrupted by the IPI handler.
Regards,
K. Y
| |