[ovs-dev] [PATCH] datapath-windows: extension fails to be enabled

[Sorin Vinturis]

Hi Nithin,

Please see my answers inline.

Thanks,
Sorin

-----Original Message-----
From: Nithin Raju [mailto:nithin at vmware.com] 
Sent: Monday, 13 April, 2015 20:55
To: Sorin Vinturis
Cc: dev at openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows: extension fails to be enabled

> On Apr 9, 2015, at 5:20 AM, Sorin Vinturis <svinturis at cloudbasesolutions.com> wrote:
> 
> The extension failed to be activated during booting due to the failure 
> to initialize tunnel filter. This happened because the Base Filtering 
> Engine
> (BFE) is not started and no session to the engine could be acquired.
> 
> The solution for this was to registered a BFE notification callback 
> that is called whenever the BFE's state changes. Only if the BFE's 
> state is running the tunnel filter is initialized.
> 
> Also, I have removed an inappropriate assert from the 
> FilterNetPnPEvent routine, OvsExtNetPnPEvent. When NDIS calls the 
> FilterNetPnPEvent routine, the extension is in paused state and, 
> obviously, the switch is not active. The switch becomes active after 
> FilterRestart routine is called and the restart is successfully complete.

Sorin,
Would you mind splitting up the patch into two parts since they are not related to each other.
SV: I'll do that.

> @@ -526,7 +523,6 @@ OvsExtNetPnPEvent(NDIS_HANDLE filterModuleContext,
>             switchContext->isActivateFailed = TRUE;
>         } else {
>             ASSERT(switchContext->isActivated == FALSE);
> -            ASSERT(switchActive == TRUE);
>             if (switchContext->isActivated == FALSE && switchActive == TRUE) {
>                 status = OvsActivateSwitch(switchContext);
>                 OVS_LOG_TRACE("OvsExtNetPnPEvent: activated switch: %p 
> “

I look at the change in FilterNetPnPEvent callback. Your change looks good, but I am also wondering if we should remove the ‘switchActive == TRUE’ check in the ‘if()’ condition.
SV: The FilterNetPnPEvent callback is called for a "NetEventSwitchActivate" NetPnpEvent notification just after FilterAttach routine call and before FilterRestart. Looking at the "Module States of a Filter Driver" picture, https://msdn.microsoft.com/en-us/library/windows/hardware/ff560558(v=vs.85).aspx, we can conclude that when NetPnpEvent notification arrives the module is in paused state. Thus I think that we cannot remove the switchActive check from the condition you have specified.

The ASSERT() is active only on debug builds anyway.

I looked at the documentation for “FilterNetPnPEvent”:
https://msdn.microsoft.com/en-us/library/ff549962(v=vs.85).aspx

And couldn’t figure out why the switch mini port would call the ‘FilterNetPnPEvent’ function. I set a breakpoint on the function and activated the switch, and didn’t see the breakpoint hit. Only the Attach and Restart breakpoints hit but not the FilterNetPnPEvent.

1. Did you see it hit?
SV: Yes, I saw when the ‘FilterNetPnPEvent’ function is called. Boot in Hyper-V, enable the extension and restart the machine. Then in the booting phase the extension is loaded, DriverEntry routine is called, then FilterAttach, FilterNetPnPEvent and then FilterRestart. This is the sequence of the calls.

2. If it was hit, was it hit after the “Restart” function? I don’t know if the current logic would work if it was hit after “Restart”. I looked the sample code provided by Microsoft, and they also have call to “init switch” without much checks for whether the “init switch” has already been called or not.
SV: No, the FilterRestart is called after the FilterNetPnPEvent call as I explained in the previous answer.

-- Nithin