> -----Original Message----- > From: Dexuan Cui > Sent: Thursday, December 10, 2015 2:19 AM > To: KY Srinivasan <kys@xxxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; > apw@xxxxxxxxxxxxx; vkuznets@xxxxxxxxxx; jasowang@xxxxxxxxxx > Subject: RE: [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in > interrupt context > > > -----Original Message----- > > From: devel [mailto:driverdev-devel-bounces@xxxxxxxxxxxxxxxxxxxxxx] On > Behalf > > Of K. Y. Srinivasan > > Sent: Friday, October 30, 2015 9:13 > > To: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx; > > vkuznets@xxxxxxxxxx; jasowang@xxxxxxxxxx > > Subject: [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in > > interrupt context > > > > From: Olaf Hering <olaf@xxxxxxxxx> > > > > 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@xxxxxxxxx> > > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx> > > --- > > 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
