Hi Sorin,
Thanks for this patch, and thanks for the refactoring.
One quick comment is that, we can probably move the new code in Random.h
to Util.h if you think we are not going to be adding any more functions to
Random.h. I am not sure if 2 more functions warrants a new file.
Also, I see a typo in the refactoring. Can you pls. address that?
-----Original Message-----
From: dev <dev-boun...@openvswitch.org> on behalf of Sorin Vinturis
<svintu...@cloudbasesolutions.com>
Date: Saturday, April 16, 2016 at 11:03 AM
To: "dev@openvswitch.org" <dev@openvswitch.org>
Subject: [ovs-dev] [PATCH] datapath-windows: Sample action support.
>This patch adds support for sampling to the OVS extension.
>
>The following flow was used for generating sample actions:
> ovs-ofctl add-flow tcp:127.0.0.1:9999 "actions=sample(
> probability=12345,collector_set_id=23456,obs_domain_id=34567,
> obs_point_id=45678)"
>
>Signed-off-by: Sorin Vinturis <svintu...@cloudbasesolutions.com>
>---
> datapath-windows/automake.mk | 1 +
> datapath-windows/ovsext/Actions.c | 182
>+++++++++++++++++++++++++++------
> datapath-windows/ovsext/Random.h | 49 +++++++++
> datapath-windows/ovsext/ovsext.vcxproj | 1 +
> 4 files changed, 200 insertions(+), 33 deletions(-)
> create mode 100644 datapath-windows/ovsext/Random.h
>
>diff --git a/datapath-windows/automake.mk b/datapath-windows/automake.mk
>index c9af806..9ff6c0b 100644
>--- a/datapath-windows/automake.mk
>+++ b/datapath-windows/automake.mk
>@@ -49,6 +49,7 @@ EXTRA_DIST += \
> datapath-windows/ovsext/PacketIO.h \
> datapath-windows/ovsext/PacketParser.c \
> datapath-windows/ovsext/PacketParser.h \
>+ datapath-windows/ovsext/Random.h \
> datapath-windows/ovsext/Recirc.c \
> datapath-windows/ovsext/Recirc.h \
> datapath-windows/ovsext/Stt.c \
>diff --git a/datapath-windows/ovsext/Actions.c
>b/datapath-windows/ovsext/Actions.c
>index cf54ae2..50e6e92 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -27,6 +27,7 @@
> #include "NetProto.h"
> #include "Offload.h"
> #include "PacketIO.h"
>+#include "Random.h"
> #include "Recirc.h"
> #include "Stt.h"
> #include "Switch.h"
>@@ -1592,6 +1593,131 @@ OvsExecuteHash(OvsFlowKey *key,
> hash = 1;
>
> key->dpHash = hash;
>+}
>+
>+/*
>+ *
>--------------------------------------------------------------------------
>+ * OvsOutputUserspaceAction --
>+ * This function sends the packet to userspace according to nested
>+ * %OVS_USERSPACE_ATTR_* attributes.
>+ *
>--------------------------------------------------------------------------
>+ */
>+static __inline NDIS_STATUS
>+OvsOutputUserspaceAction(OvsForwardingContext *ovsFwdCtx,
>+ OvsFlowKey *key,
>+ const PNL_ATTR attr)
>+{
>+ NTSTATUS status = NDIS_STATUS_SUCCESS;
>+ PNL_ATTR userdataAttr;
>+ PNL_ATTR queueAttr;
>+ POVS_PACKET_QUEUE_ELEM elem;
>+ POVS_PACKET_HDR_INFO layers = &ovsFwdCtx->layers;
>+ BOOLEAN isRecv = FALSE;
>+
>+ POVS_VPORT_ENTRY vport =
>OvsFindVportByPortNo(ovsFwdCtx->switchContext,
>+ ovsFwdCtx->srcVportNo);
>+
>+ if (vport) {
>+ if (vport->isExternal ||
>+ OvsIsTunnelVportType(vport->ovsType)) {
>+ isRecv = TRUE;
>+ }
>+ }
>+
>+ queueAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_PID);
>+ userdataAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_USERDATA);
>+
>+ elem = OvsCreateQueueNlPacket(NlAttrData(userdataAttr),
>+ NlAttrGetSize(userdataAttr),
The existing code passes ŒuserdataAttr¹ as-is and not the payload.
Basically, it passes the NL_ATTR structure and not just the payload. Any
reason to strip off the header?
If you follow through to how OvsCreateQueueNlPacket() uses this, you¹ll
end up with NlMsgPutTailUnspec() and that needs the NL_ATTR header to be
present as part of the data.
>+ OVS_PACKET_CMD_ACTION,
>+ vport, key, ovsFwdCtx->curNbl,
>+
>NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl),
>+ isRecv,
>+ layers);
>+ if (elem) {
>+ LIST_ENTRY missedPackets;
>+ InitializeListHead(&missedPackets);
>+ InsertTailList(&missedPackets, &elem->link);
>+ OvsQueuePackets(&missedPackets, 1);
>+ } else {
>+ status = NDIS_STATUS_FAILURE;
>+ }
>+
>+ return status;
>+}
>+
>+/*
>+ *
>--------------------------------------------------------------------------
>+ * OvsExecuteSampleAction --
>+ * Executes actions based on probability, as specified in the nested
>+ * %OVS_SAMPLE_ATTR_* attributes.
>+ *
>--------------------------------------------------------------------------
>+ */
>+static __inline NDIS_STATUS
>+OvsExecuteSampleAction(OvsForwardingContext *ovsFwdCtx,
>+ OvsFlowKey *key,
>+ const PNL_ATTR attr)
>+{
>+ PNET_BUFFER_LIST newNbl = NULL;
>+ PNL_ATTR actionsList = NULL;
>+ PNL_ATTR a = NULL;
>+ INT rem = 0;
>+
>+ SRand();
>+ NL_ATTR_FOR_EACH_UNSAFE(a, rem, NlAttrData(attr),
>NlAttrGetSize(attr)) {
>+ switch (NlAttrType(a)) {
>+ case OVS_SAMPLE_ATTR_PROBABILITY:
>+ {
>+ UINT32 probability = NlAttrGetU32(a);
>+
>+ if (!probability || Rand() > probability) {
>+ return 0;
>+ }
>+ break;
>+ }
>+ case OVS_SAMPLE_ATTR_ACTIONS:
>+ actionsList = a;
>+ break;
>+ }
>+ }
>+
>+ if (actionsList) {
>+ rem = NlAttrGetSize(actionsList);
>+ a = (PNL_ATTR)NlAttrData(actionsList);
>+ }
>+
>+ if (!rem) {
>+ /* Actions list is empty, do nothing */
>+ return STATUS_SUCCESS;
>+ }
>+
>+ /*
>+ * The only known usage of sample action is having a single
>user-space
>+ * action. Treat this usage as a special case.
>+ */
>+ if (NlAttrType(a) == OVS_ACTION_ATTR_USERSPACE &&
>+ NlAttrIsLast(a, rem)) {
>+ return OvsOutputUserspaceAction(ovsFwdCtx, key, a);
>+ }
>+
>+ newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext,
>ovsFwdCtx->curNbl,
>+ 0, 0, TRUE /*copy NBL info*/);
>+ if (newNbl == NULL) {
>+ /*
>+ * Skip the sample action when out of memory, but continue on
>with the
>+ * rest of the action list.
>+ */
>+ ovsActionStats.noCopiedNbl++;
>+ return STATUS_SUCCESS;
>+ }
>+
>+ if (!OvsAddDeferredActions(newNbl, key, a)) {
>+ OVS_LOG_INFO(
>+ "Deferred actions limit reached, dropping sample action.");
>+ OvsCompleteNBL(ovsFwdCtx->switchContext, newNbl, TRUE);
>+ }
>+
>+ return STATUS_SUCCESS;
> }
>
> /*
>@@ -1834,43 +1960,15 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT
>switchContext,
>
> case OVS_ACTION_ATTR_USERSPACE:
> {
>- PNL_ATTR userdataAttr;
>- PNL_ATTR queueAttr;
>- POVS_PACKET_QUEUE_ELEM elem;
>- BOOLEAN isRecv = FALSE;
>-
>- POVS_VPORT_ENTRY vport = OvsFindVportByPortNo(switchContext,
>- portNo);
>-
>- if (vport) {
>- if (vport->isExternal ||
>- OvsIsTunnelVportType(vport->ovsType)) {
>- isRecv = TRUE;
>- }
>- }
>-
>- queueAttr = NlAttrFindNested(a, OVS_USERSPACE_ATTR_PID);
>- userdataAttr = NlAttrFindNested(a,
>OVS_USERSPACE_ATTR_USERDATA);
>-
>- elem = OvsCreateQueueNlPacket((PVOID)userdataAttr,
>- userdataAttr->nlaLen,
>- OVS_PACKET_CMD_ACTION,
>- vport, key, ovsFwdCtx.curNbl,
>-
>NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx.curNbl),
>- isRecv,
>- layers);
>- if (elem) {
>- LIST_ENTRY missedPackets;
>- InitializeListHead(&missedPackets);
>- InsertTailList(&missedPackets, &elem->link);
>- OvsQueuePackets(&missedPackets, 1);
>- dropReason = L"OVS-Completed since packet was copied to "
>- L"userspace";
>- } else {
>+ status = OvsOutputUserspaceAction(&ovsFwdCtx, key,
>+ (const PNL_ATTR)a);
>+ if (status != NDIS_STATUS_SUCCESS) {
> dropReason = L"OVS-Dropped due to failure to queue to "
> L"userspace";
> goto dropit;
> }
>+ dropReason = L"OVS-Completed since packet was copied to "
>+ L"userspace";
> break;
> }
> case OVS_ACTION_ATTR_SET:
>@@ -1894,6 +1992,24 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT
>switchContext,
> break;
> }
> case OVS_ACTION_ATTR_SAMPLE:
>+ {
>+ if (ovsFwdCtx.destPortsSizeOut > 0 || ovsFwdCtx.tunnelTxNic
>!= NULL
>+ || ovsFwdCtx.tunnelRxNic != NULL) {
>+ status = OvsOutputBeforeSetAction(&ovsFwdCtx);
>+ if (status != NDIS_STATUS_SUCCESS) {
>+ dropReason = L"OVS-adding destination failed";
>+ goto dropit;
>+ }
>+ }
>+
>+ status = OvsExecuteSampleAction(&ovsFwdCtx, key,
>+ (const PNL_ATTR)a);
>+ if (status != NDIS_STATUS_SUCCESS) {
>+ dropReason = L"OVS-sample action failed";
>+ goto dropit;
>+ }
>+ break;
>+ }
> default:
> status = NDIS_STATUS_NOT_SUPPORTED;
> break;
>diff --git a/datapath-windows/ovsext/Random.h
>b/datapath-windows/ovsext/Random.h
>new file mode 100644
>index 0000000..d8cc3d5
>--- /dev/null
>+++ b/datapath-windows/ovsext/Random.h
>@@ -0,0 +1,49 @@
>+/*
>+ * Copyright (c) 2016 Cloudbase Solutions Srl
>+ *
>+ * Licensed under the Apache License, Version 2.0 (the "License");
>+ * you may not use this file except in compliance with the License.
>+ * You may obtain a copy of the License at:
>+ *
>+ *
>https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_license
>s_LICENSE-2D2.0&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=p
>NHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=_S_KHL11G8R5egTOUkbQM8aNCItgc
>V0Y_xki2GNLLco&s=nvx_La1EmdtV5AyFBFNO_xRh6AIvlU5KvKsTzRG1obU&e=
>+ *
>+ * Unless required by applicable law or agreed to in writing, software
>+ * distributed under the License is distributed on an "AS IS" BASIS,
>+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>implied.
>+ * See the License for the specific language governing permissions and
>+ * limitations under the License.
>+ */
>+
>+#ifndef __RANDOM_H_
>+#define __RANDOM_H_ 1
>+
>+#include "precomp.h"
>+
>+static LARGE_INTEGER seed;
>+
>+/*
>+
>*-------------------------------------------------------------------------
>---
>+ * SRand --
>+ * This function sets the starting seed value for the pseudorandom
>number
>+ * generator.
>+
>*-------------------------------------------------------------------------
>---
>+ */
>+static __inline
>+VOID SRand()
>+{
>+ KeQuerySystemTime(&seed);
>+}
>+
>+/*
>+
>*-------------------------------------------------------------------------
>---
>+ * Rand --
>+ * This function generates a pseudorandom number between 0 to
>UINT_MAX.
>+
>*-------------------------------------------------------------------------
>---
>+ */
>+static __inline
>+UINT32 Rand()
>+{
>+ return seed.LowPart *= 0x8088405 + 1;
>+}
>+
>+#endif /* __RANDOM_H_ */
>diff --git a/datapath-windows/ovsext/ovsext.vcxproj
>b/datapath-windows/ovsext/ovsext.vcxproj
>index 0356ddf..8174dbe 100644
>--- a/datapath-windows/ovsext/ovsext.vcxproj
>+++ b/datapath-windows/ovsext/ovsext.vcxproj
>@@ -95,6 +95,7 @@
> <ClInclude Include="PacketIO.h" />
> <ClInclude Include="PacketParser.h" />
> <ClInclude Include="precomp.h" />
>+ <ClInclude Include="Random.h" />
> <ClInclude Include="Recirc.h" />
> <ClInclude Include="resource.h" />
> <ClInclude Include="Stt.h" />
>--
>1.9.0.msysgit.0
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma
>n_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pN
>HQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=_S_KHL11G8R5egTOUkbQM8aNCItgcV
>0Y_xki2GNLLco&s=yoHzEzE_vb4SwDiJ8fpI1hRMHiBu6CK1orqjYba1dLM&e=
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
