[ovs-dev] [PATCH 2/2] datapath-windows: Support for IRP cancelling mechanism
Sorin Vinturis
svinturis at cloudbasesolutions.com
Tue Aug 4 11:02:17 UTC 2015
Nithin, thanks for reviewing this patch. There is a v2 version of this patch though. Please see my answers inline.
-----Original Message-----
From: Nithin Raju [mailto:nithin at vmware.com]
Sent: Monday, 3 August, 2015 19:56
To: Sorin Vinturis
Cc: dev at openvswitch.org
Subject: Re: [ovs-dev] [PATCH 2/2] datapath-windows: Support for IRP cancelling mechanism
hi Sorin,
Thanks for this patch. This is a functionality that is needed to complete the tunnel filter async request story.
I have a few comments that are inlined.
How did you test this patch, BTW?
> On Jul 15, 2015, at 1:55 AM, Sorin Vinturis <svinturis at cloudbasesolutions.com> wrote:
>
> Under certain circumstances, we might need to cancel a pending IRP
> that has been submitted and not yet responded. This might occur when
> the request takes too long to complete or when the process which
> initiated the request terminated, leaving the request outstanding.
>
> This patch adds this missing piece by providing support for IRP
> cancelling mechanism.
>
> Signed-off-by: Sorin Vinturis <svinturis at cloudbasesolutions.com>
> Reported-by: Sorin Vinturis <svinturis at cloudbasesolutions.com>
> Reported-at:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvs
> witch_ovs-2Dissues_issues_95&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-
> YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=YvyQyGf7
> otxtNV6G70r2mB7itwkZaaeB-EXrNpgI8mY&s=0s5p1RLhoeCYNlLIT6ydI0fIQkvg1Pxp
> eRyV_tTzAW4&e=
> ---
> datapath-windows/ovsext/Datapath.c | 4 ++
> datapath-windows/ovsext/TunnelFilter.c | 110 +++++++++++++++++++++++++++++++--
> datapath-windows/ovsext/TunnelIntf.h | 3 +
> 3 files changed, 112 insertions(+), 5 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Datapath.c
> b/datapath-windows/ovsext/Datapath.c
> index 4af909c..0ec33bd 100644
> --- a/datapath-windows/ovsext/Datapath.c
> +++ b/datapath-windows/ovsext/Datapath.c
> @@ -922,6 +922,10 @@ exit:
> * to be processed later, so we mark the IRP as pending and complete
> * it in another thread when the request is processed. */
> IoMarkIrpPending(irp);
> +
> + /* Set the Cancel routine for the pending IRP. */
> + IoSetCancelRoutine(irp, OvsTunnelFilterCancelIrp);
I think it would be best in terms of code maintainability if we do "IoSetCancelRoutine(irp, OvsTunnelFilterCancelIrp);”, in OvsInitVxlanTunnel() or OvsInitTunnelVport(). Also, I feel it is better to setup the cancel routing before handing off the IRP to the tunnel filter worker thread. ie. before the calls into OvsTunnelFilterCreate() and OvsTunnelFilterDelete().
SV: There is a v2 version of this patch. The setting of the Cancel routine is done before pushing the request to the thread queue, when the IRP is marked as pending.
> +
> return status;
> }
> return OvsCompleteIrpRequest(irp, (ULONG_PTR)replyLen, status);
> diff --git a/datapath-windows/ovsext/TunnelFilter.c
> b/datapath-windows/ovsext/TunnelFilter.c
> index caaf3f6..2542f63 100644
> --- a/datapath-windows/ovsext/TunnelFilter.c
> +++ b/datapath-windows/ovsext/TunnelFilter.c
> @@ -168,6 +168,8 @@ static VOID OvsTunnelFilterThreadStop(POVS_TUNFLT_THREAD_CONTEXT threadCtx,
> BOOLEAN signalEvent); static
> NTSTATUS OvsTunnelFilterThreadInit(POVS_TUNFLT_THREAD_CONTEXT threadCtx);
> static VOID OvsTunnelFilterThreadUninit(POVS_TUNFLT_THREAD_CONTEXT threadCtx);
> +static VOID OvsTunnelFilterSetIrpContext(POVS_TUNFLT_REQUEST_LIST listRequests,
> + POVS_TUNFLT_REQUEST
> +request);
>
> /*
> * Callout driver global variables
> @@ -983,6 +985,7 @@ VOID
> OvsTunnelFilterRequestPush(POVS_TUNFLT_REQUEST_LIST listRequests,
> POVS_TUNFLT_REQUEST request) {
> +
> NdisAcquireSpinLock(&listRequests->spinlock);
>
> InsertTailList(&listRequests->head, &(request->entry)); @@ -1012,6
> +1015,10 @@ OvsTunnelFilterThreadPush(POVS_TUNFLT_REQUEST request)
> &gTunnelThreadCtx[threadIndex].listRequests,
> request);
>
> + OvsTunnelFilterSetIrpContext(
> + &gTunnelThreadCtx[threadIndex].listRequests,
> + request);
> +
> KeSetEvent(&gTunnelThreadCtx[threadIndex].requestEvent,
> IO_NO_INCREMENT,
> FALSE);
> @@ -1062,6 +1069,20 @@ OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
> while (NULL != (request = OvsTunnelFilterRequestPop(
> &threadCtx->listRequests))) {
>
> + if (request->irp) {
> + if (!IoSetCancelRoutine(request->irp, NULL)) {
> + /*
> + * The Cancel routine for the current IRP is running.
> + * Leave the IRP alone and go to the next one.
> + */
> + continue;
> + }
From the documentation of IoSetCancelRoutine(), it seems that it is best to setup the cancel routine before we start processing.
"If no Cancel routine was previously set, or if IRP cancellation is already in progress, IoSetCancelRoutine returns NULL.”
SV: The setting of the Cancel routine is done before processing the request, when the new request is pushed to the thread queue. Please see the v2 version of this patch.
If you are setting up the cancel routing at the tail of OvsDeviceControl(), there’s a potential race condition between the system call context and the tunnel filter thread context.
SV: That is not the case. Please see the v2 patch.
> + }
> +
> + /*
> + * The Cancel routine cannot run now and cannot already have
> + * started to run.
> + */
> status = OvsTunnelFilterExecuteAction(threadCtx->engineSession,
> request);
>
> @@ -1321,7 +1342,7 @@
> OvsTunnelFilterThreadUninit(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
> *
> ----------------------------------------------------------------------
> ----
> * This function creates a new tunnel filter request and push it to a
> thread
> * queue. If the thread stop event is signaled, the request is
> completed with
> - * STATUS_CANCELLED without pushing it to any queue.
> + * STATUS_REQUEST_ABORTED without pushing it to any queue.
> *
> ----------------------------------------------------------------------
> ----
> */
> NTSTATUS
> @@ -1347,7 +1368,7 @@ OvsTunnelFilterQueueRequest(PIRP irp,
> (LARGE_INTEGER *)&timeout)) {
> /* The stop event is signaled. Completed the IRP with
> * STATUS_CANCELLED. */
> - status = STATUS_CANCELLED;
> + status = STATUS_REQUEST_ABORTED;
> break;
> }
>
> @@ -1396,8 +1417,8 @@ OvsTunnelFilterQueueRequest(PIRP irp,
>
> /*
> *
> ----------------------------------------------------------------------
> ----
> - * This function adds a new WFP filter for the received port and
> returns the
> - * ID of the created WFP filter.
> + * This function adds a new WFP filter for the received port and
> + returns the
> + * ID of the created WFP filter.
> *
> * Note:
> * All necessary calls to the WFP filtering engine must be running at
> IRQL = @@ -1436,7 +1457,7 @@ OvsTunelFilterCreate(PIRP irp,
>
> /*
> *
> ----------------------------------------------------------------------
> ----
> - * This function removes a WFP filter using the received filter ID.
> + * This function removes a WFP filter using the received filter ID.
> *
> * Note:
> * All necessary calls to the WFP filtering engine must be running at
> IRQL = @@ -1471,3 +1492,82 @@ OvsTunelFilterDelete(PIRP irp,
> callback,
> tunnelContext); }
> +
> +VOID
> +OvsTunnelFilterSetIrpContext(POVS_TUNFLT_REQUEST_LIST listRequests,
> + POVS_TUNFLT_REQUEST request) {
> + PIRP irp = request->irp;
> +
> + if (irp) {
> + /* Set the IRP's DriverContext to be used for later. */
> + irp->Tail.Overlay.DriverContext[0] = (PVOID)request;
> + irp->Tail.Overlay.DriverContext[1] = (PVOID)listRequests;
> + }
> +}
> +
> +/*
> + *
> +---------------------------------------------------------------------
> +-----
> + * This function removes the cancelled request from the corresponding
> +thread
> + * queue.
> + *
> +---------------------------------------------------------------------
> +-----
> + */
> +VOID
> +OvsTunnelFilterRequestDequeue(PIRP irp) {
> + POVS_TUNFLT_REQUEST_LIST listRequests =
> + (POVS_TUNFLT_REQUEST_LIST)irp->Tail.Overlay.DriverContext[1];
> + PLIST_ENTRY head, link, next;
> +
> + NdisAcquireSpinLock(&listRequests->spinlock);
> +
> + head = &listRequests->head;
> + LIST_FORALL_SAFE(head, link, next) {
> + POVS_TUNFLT_REQUEST request = NULL;
> +
> + request = CONTAINING_RECORD(link, OVS_TUNFLT_REQUEST, entry);
> +
> + if (request->irp == irp) {
> + RemoveEntryList(&request->entry);
> + listRequests->numEntries--;
> + break;
> + }
> + }
> +
> + NdisReleaseSpinLock(&listRequests->spinlock);
> +}
> +
> +/*
> + *
> +---------------------------------------------------------------------
> +-----
> + * This function is the Cancel routine to be called if the IRP is canceled.
> + *
> +---------------------------------------------------------------------
> +-----
> + */
> +VOID
> +OvsTunnelFilterCancelIrp(PDEVICE_OBJECT DeviceObject,
> + PIRP irp)
> +{
> + POVS_TUNFLT_REQUEST request =
> + (POVS_TUNFLT_REQUEST)irp->Tail.Overlay.DriverContext[0];
> +
> + DBG_UNREFERENCED_PARAMETER(DeviceObject);
> +
> + /* Release the global cancel spinlock. */
> + IoReleaseCancelSpinLock(irp->CancelIrql);
Can we call IoReleaseCancelSpinLock() if we have not called IoAcquireCancelSpinLock()? Who calls into IoAcquireCancelSpinLock()?
SV: The Cancel spinlock is held by the I/O Manager before calling the Cancel routine. Thus the call to release it in out Cancel routine.
The documentation for IoSetCancelRoutine() indicates the following:
"A driver must hold the system cancel spin lock when calling this routine if the driver uses the I/O manager-supplied device queue in the device object. The driver runs at IRQL = DISPATCH_LEVEL after calling IoAcquireCancelSpinLock until it releases the cancel spin lock withIoReleaseCancelSpinLock."
SV: This is correct. We are not using the I/O manager-supplied device queue for managing pending IRPs. We are using our own queue, with own spinlock, thus we don't need the system cancel spin lock to be held and that is why we are releasing it.
> +
> + /*
> + * Remove the request from the corresponding tunnel filter thread's
> + * queue.
> + */
> + OvsTunnelFilterRequestDequeue(irp);
We have a pointer to the ‘request’. Can’t we just do? Do we need a separate function?
SV: Yes, you are right. This is how is made in the v2 version is this patch.
NdisAcquireSpinLock(&listRequests->spinlock);
RemoveEntryList(&request->entry);
listRequests->numEntries—;
NdisReleaseSpinLock(&listRequests->spinlock);
thanks,
-- Nithin
More information about the dev
mailing list