[ovs-dev] [PATCH] datapath-windows: Incorrect assumption of the IRQL

[Nithin Raju]

hi Sorin,
I had minor comments. Pls. see inline.

> #define NETLINK_FAMILY_NAME_LEN 48
> 
> +#ifndef IFNAMSIZ
> +#define IFNAMSIZ 16
> +#endif

This is defined in OvsDpInterface.h. We can avoid an extra definition. See commit - f92156ae2c86cf1b7c2076f2bca88b6fa10ec632.

> +
> 
> /*
>  * Netlink messages are grouped by family (aka type), and each family supports
> @@ -1398,7 +1402,8 @@ OvsGetVportDumpNext(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
>      * ATM we assume we have one pid only.
>     */
> 
> -    NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState, 0);
> +    NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState,
> +        NDIS_RWL_AT_DISPATCH_LEVEL);
> 
>     if (gOvsSwitchContext->numVports > 0) {
>         /* inBucket: the bucket, used for lookup */
> diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c
> index 5cab6e1..1c0111f 100644
> --- a/datapath-windows/ovsext/Flow.c
> +++ b/datapath-windows/ovsext/Flow.c
> @@ -1150,7 +1150,7 @@ OvsDoDumpFlows(OvsFlowDumpInput *dumpInput,
> 
>     datapath = &gOvsSwitchContext->datapath;
>     ASSERT(datapath);
> -    OvsAcquireDatapathRead(datapath, &dpLockState, FALSE);
> +    OvsAcquireDatapathRead(datapath, &dpLockState, TRUE);


We can add such an ASSERT whereever we have inferred that we are at dispatch level. ASSERT would make sure that if we remove the Ctrl Lock in the future, then we'll not make wrong assumption about the dispatch level.

> 
>     head = &datapath->flowTable[rowIndex];
>     node = head->Flink;
> @@ -1297,7 +1297,7 @@ OvsPutFlowIoctl(PVOID inputBuffer,
> 
>     datapath = &gOvsSwitchContext->datapath;
>     ASSERT(datapath);
> -    OvsAcquireDatapathWrite(datapath, &dpLockState, FALSE);
> +    OvsAcquireDatapathWrite(datapath, &dpLockState, TRUE);
>     status = HandleFlowPut(put, datapath, stats);
>     OvsReleaseDatapath(datapath, &dpLockState);
> 
> @@ -1489,7 +1489,7 @@ OvsGetFlowIoctl(PVOID inputBuffer,
> 
>     datapath = &gOvsSwitchContext->datapath;
>     ASSERT(datapath);
> -    OvsAcquireDatapathRead(datapath, &dpLockState, FALSE);
> +    OvsAcquireDatapathRead(datapath, &dpLockState, TRUE);
>     flow = OvsLookupFlow(datapath, &getInput->key, &hash, FALSE);
>     if (!flow) {
>         status = STATUS_INVALID_PARAMETER;
> @@ -1524,7 +1524,7 @@ OvsFlushFlowIoctl(UINT32 dpNo)
> 
>     datapath = &gOvsSwitchContext->datapath;
>     ASSERT(datapath);
> -    OvsAcquireDatapathWrite(datapath, &dpLockState, FALSE);
> +    OvsAcquireDatapathWrite(datapath, &dpLockState, TRUE);
>     DeleteAllFlows(datapath);
>     OvsReleaseDatapath(datapath, &dpLockState);
> 
> diff --git a/datapath-windows/ovsext/PacketIO.c b/datapath-windows/ovsext/PacketIO.c
> index ac7862d..a0e096a 100644
> --- a/datapath-windows/ovsext/PacketIO.c
> +++ b/datapath-windows/ovsext/PacketIO.c
> @@ -267,7 +267,6 @@ OvsStartNBLIngress(POVS_SWITCH_CONTEXT switchContext,
>                 goto dropit;
>             }
> 
> -            ASSERT(KeGetCurrentIrql() == DISPATCH_LEVEL);
>             OvsAcquireDatapathRead(datapath, &dpLockState, dispatch);

We can pass NDIS_RWL_AT_DISPATCH_LEVEL instead of 'dispatch', and let the ASSERT be.

> 
>             flow = OvsLookupFlow(datapath, &key, &hash, FALSE);
> diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/User.c
> index 26cd431..d6ad650 100644
> --- a/datapath-windows/ovsext/User.c
> +++ b/datapath-windows/ovsext/User.c
> @@ -365,7 +365,6 @@ OvsExecuteDpIoctl(PVOID inputBuffer,
>     ndisStatus = OvsExtractFlow(pNbl, fwdDetail->SourcePortId, &key, &layers,
>                               NULL);
>     if (ndisStatus == NDIS_STATUS_SUCCESS) {
> -        ASSERT(KeGetCurrentIrql() == DISPATCH_LEVEL);
>         NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState,
>                               NDIS_RWL_AT_DISPATCH_LEVEL);

IMO, the ASSERT it harmless and can stay. Esp. since we are passing NDIS_RWL_AT_DISPATCH_LEVEL as the argument.

thanks,
-- Nithin