I spent some time today reviewing the Cloudbase Windows kernel module
repository. I have some preliminary comments, even though I have not
read the whole thing yet.
I feel a little funny reviewing this repository, because it is not
currently proposed to become part of the Open vSwitch tree--although I
think that that would be the ideal outcome--and because I do not know
Windows well. I am reviewing it anyway because it might make sense as
part of the OVS tree in the future and because its quality will
directly affect the usability of Open vSwitch on Hyper-V should the
corresponding userspace portions be adopted.
If this is destined to become part of the Open vSwitch tree, then we'd
need Signed-off-by:s for the patches.
Some of the code looks like it is derived from Microsoft sample code.
Much of the code in Cloudbase's Driver/Core/NdisFilter.c strongly
resembles code in the Microsoft sample base/SxBase.c, and similarly
for Driver/OID/OIDRequest.c and base/SxLibrary.c. I do not see
Microsoft copyright notice and license text on those file, but they
should be there.
The kernel code seems to have a fixed notion of what an Ethernet
packet may contain (e.g. see PacketInfo_Extract() and
VerifyNetBuffer()), and rejects anything that it doesn't understand.
I believe that this makes it impossible to implement a plain L2 switch
that forwards all packets. (That is one reason why the other Open
vSwitch datapaths do not verify packet contents.) It also means that
Open vSwitch on Hyper-V can't support protocols not explicitly
mentioned in the kernel module, such as CFM or STP.
Most of the license statements have a typo in the URL to the Apache
license: they say "http :" instead of "http:".
It is strange to have so many references to OpenFlow and "OF" prefixes
in the code, because an Open vSwitch datapath does not directly
implement OpenFlow. Other Open vSwitch datapaths do not talk about
OpenFlow for that reason.
Does anything verify that argument values are present? When I look at
OFPort_New(), for example, I see many checks that arguments are
present, but I do not see anything that makes sure that the values
attached to them are present and of the expected size. It looks to me
like the values are blindly dereferenced.
The code to parse netlink arguments is very verbose. If you look at
the userspace netlink code, it uses an "nl_policy" mechanism to make
parsing more succinct, more systematic, and possibly even more
efficient. Did you consider that approach?
I see lots of cut-and-pasted code duplication. For example,
Flow_New() and Flow_Set(), in WinlFlow.c, have probably 75% or more of
their approximately 200 lines in common. I think that's going to make
this code difficult to maintain and to completely review.
The code is very wide. I see many lines over 100 characters, and
several that are almost 200 characters wide. Do you have any
guidelines on width?
The code is very verbose. A simple if-else spans 9 lines:
if (pFlow)
{
++pDatapath->statistics.flowTableMatches;
}
else
{
++pDatapath->statistics.flowTableMissed;
}
although I do often see the extra blank line omitted before the 'else'.
Do you find this style easy to read?
I'm surprised that Flow_Dump() dumps all the flows in one go. That
could be hundreds of thousands of flows. Other datapaths send flows
to userspace in batches.
I think that I see the ingress code making a full copy of every
packet's data. It also looks like there's a full copy for each
output. That's pretty expensive, does Windows have some kind of
reference count and copy-on-write mechanism to avoid it?
If I'm reading the code correctly, the flow table search is linear.
This is too expensive to be practical in production.
It looks like _ExecuteAction_Sample() returns "failure", so that if
the sample is not selected, no actions following the sample action are
executed. That's not the expected behavior: whether the sample is
selected should only affect whether the *nested* actions are executed
and should have no effect on whether the *following* actions are
executed.
It looks like _ExecuteAction_Sample() interprets the probability as a
percent. It is not. It is a 32-bit fraction, that is, UINT32_MAX/2
would be a 50% probability, UINT32_MAX/3 would be a 33% probability,
and so on.
The "push vlan" action should always push a VLAN tag. It looks like
Vlan_Push() only pushes a VLAN tag if there is not already one.
It looks like Vlan_Push() always sets the CFI bit in the 802.1Q
header. It should never set the CFI bit.
It looks like Vlan_Pop() fails on a double-tagged VLAN packet. It
should not fail.
I think that you could just drop the "priority" and "mark" support. I
don't think they're useful on Windows.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
