Hi Sairam, Thanks for the review, please find my response inline. I send out a v2 addressing review comments.
Regards, Anand Kumar On 8/24/18, 2:58 PM, "Sairam Venugopal" <[email protected]> wrote: Hi Anand, Thanks for the patch. See comments inline. Thanks, Sairam On 8/21/18, 2:58 PM, "[email protected] on behalf of Anand Kumar" <[email protected] on behalf of [email protected]> wrote: This patch implements limiting conntrack entries per zone using dpctl commands. Example: ovs-appctl dpctl/ct-set-limits default=5 zone=1,limit=2 zone=1,limit=3 ovs-appctl dpct/ct-del-limits zone=4 ovs-appctl dpct/ct-get-limits zone=1,2,3 - Also update the netlink-socket.c to support netlink family 'OVS_WIN_NL_CTLIMIT_FAMILY_ID' for conntrack zone limit. Signed-off-by: Anand Kumar <[email protected]> --- datapath-windows/include/OvsDpInterfaceExt.h | 1 + datapath-windows/ovsext/Conntrack.c | 163 ++++++++++++++++++++++++++- datapath-windows/ovsext/Conntrack.h | 12 ++ datapath-windows/ovsext/Datapath.c | 34 +++++- lib/netlink-socket.c | 5 + 5 files changed, 212 insertions(+), 3 deletions(-) diff --git a/datapath-windows/include/OvsDpInterfaceExt.h b/datapath-windows/include/OvsDpInterfaceExt.h index db91c3e..5fd8000 100644 --- a/datapath-windows/include/OvsDpInterfaceExt.h +++ b/datapath-windows/include/OvsDpInterfaceExt.h @@ -72,6 +72,7 @@ */ #define OVS_WIN_NL_CT_FAMILY_ID (NLMSG_MIN_TYPE + 7) +#define OVS_WIN_NL_CTLIMIT_FAMILY_ID (NLMSG_MIN_TYPE + 8) #define OVS_WIN_NL_INVALID_MCGRP_ID 0 #define OVS_WIN_NL_MCGRP_START_ID 100 diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c index dd16602..b806cd7 100644 --- a/datapath-windows/ovsext/Conntrack.c +++ b/datapath-windows/ovsext/Conntrack.c @@ -34,6 +34,8 @@ static OVS_CT_THREAD_CTX ctThreadCtx; static PNDIS_RW_LOCK_EX *ovsCtBucketLock = NULL; extern POVS_SWITCH_CONTEXT gOvsSwitchContext; static ULONG ctTotalEntries; +static POVS_CT_ZONE_INFO zoneInfo = NULL; +static ULONG defaultCtLimit; static __inline OvsCtFlush(UINT16 zone, struct ovs_key_ct_tuple_ipv4 *tuple); static __inline NDIS_STATUS @@ -99,6 +101,19 @@ OvsInitConntrack(POVS_SWITCH_CONTEXT context) if (status != STATUS_SUCCESS) { OvsCleanupConntrack(); } + Sai: Can you move the following prior to the OvsNatInit or handle OvsCleanupConntrack()? Also, shouldn't zoneInfo have a lock for manipulation? [AK]: Sure, I will move it before OvsNatInit(). A lock is needed only when there are multiple calls to manipulate same zone id/defaults in parallel. I will handle this with a spinlock in v2 patch. + zoneInfo = OvsAllocateMemoryWithTag(sizeof(OVS_CT_ZONE_INFO) * + (UINT16_MAX + 1), OVS_CT_POOL_TAG); + if (zoneInfo == NULL) { + status = STATUS_INSUFFICIENT_RESOURCES; + goto freeBucketLock; + } + + defaultCtLimit = CT_MAX_ENTRIES; + for (int i = 0; i <= UINT16_MAX; i++) { + zoneInfo[i].entries = 0; + zoneInfo[i].limit = defaultCtLimit; + } return STATUS_SUCCESS; freeBucketLock: @@ -149,6 +164,22 @@ OvsCleanupConntrack(VOID) OvsFreeMemoryWithTag(ovsCtBucketLock, OVS_CT_POOL_TAG); ovsCtBucketLock = NULL; OvsNatCleanup(); + if (zoneInfo) { + OvsFreeMemoryWithTag(zoneInfo, OVS_CT_POOL_TAG); + } +} + +VOID Sai: can we set zone to UINT16 instead of int? [AK]: zone cannot be UINT16 since it is set to -1 when ct-set-limits is called with a default argument. +OvsCtSetZoneLimit(int zone, ULONG value) { + if (zone == -1) { + /* Set default limit for all zones. */ + defaultCtLimit = value; + for (UINT32 i = 0; i <= UINT16_MAX; i++) { + zoneInfo[i].limit = value; + } + } else { + zoneInfo[(UINT16)zone].limit = value; + } } /* @@ -263,6 +294,7 @@ OvsCtAddEntry(POVS_CT_ENTRY entry, &entry->link); NdisInterlockedIncrement((PLONG)&ctTotalEntries); + zoneInfo[ctx->key.zone].entries++; NdisReleaseRWLock(ovsCtBucketLock[bucketIdx], &lockState); return TRUE; } @@ -437,6 +469,7 @@ OvsCtEntryDelete(POVS_CT_ENTRY entry, BOOLEAN forceDelete) if (entry->natInfo.natAction) { OvsNatDeleteKey(&entry->key); } + zoneInfo[entry->key.zone].entries--; OvsPostCtEventEntry(entry, OVS_EVENT_CT_DELETE); RemoveEntryList(&entry->link); OVS_RELEASE_SPIN_LOCK(&(entry->lock), irql); @@ -877,12 +910,16 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, &entryCreated); } else { - if (commit && ctTotalEntries >= CT_MAX_ENTRIES) { + if (commit && (ctTotalEntries >= CT_MAX_ENTRIES || + zoneInfo[ctx.key.zone].entries >= zoneInfo[ctx.key.zone].limit)) { /* Don't proceed with processing if the max limit has been hit. * This blocks only new entries from being created and doesn't * affect existing connections. */ - OVS_LOG_ERROR("Conntrack Limit hit: %lu", ctTotalEntries); + OVS_LOG_ERROR("Conntrack Limit hit: zone(%u), zoneLimit(%lu)," + "zoneEntries(%lu), ctTotalEntries(%lu),", zone, + zoneInfo[ctx.key.zone].limit, + zoneInfo[ctx.key.zone].entries, ctTotalEntries); return NDIS_STATUS_RESOURCES; } /* If no matching entry was found, create one and add New state */ @@ -1783,4 +1820,126 @@ OvsCtDumpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, return STATUS_SUCCESS; } +static NTSTATUS +OvsCreateNlMsgFromCtLimit(POVS_MESSAGE msgIn, + PVOID outBuffer, + UINT32 outBufLen, + PCHAR attr, + UINT32 numAttrs, + int dpIfIndex) +{ + NTSTATUS status = STATUS_SUCCESS; + NL_BUFFER nlBuffer; + BOOLEAN ok; + PNL_MSG_HDR nlMsg; + PGENL_MSG_HDR genlMsgHdr = &(msgIn->genlMsg); + + NlBufInit(&nlBuffer, outBuffer, outBufLen); + + ok = NlFillOvsMsg(&nlBuffer, msgIn->nlMsg.nlmsgType, NLM_F_MULTI, + msgIn->nlMsg.nlmsgSeq, msgIn->nlMsg.nlmsgPid, + msgIn->genlMsg.cmd, msgIn->genlMsg.version, + dpIfIndex); + if (!ok) { + return STATUS_INVALID_BUFFER_SIZE; + } + + if (genlMsgHdr->cmd == OVS_CT_LIMIT_CMD_GET && numAttrs) { + POVS_CT_ZONE_LIMIT zoneLimitAttr = (POVS_CT_ZONE_LIMIT) attr; + UINT32 offset = NlMsgStartNested(&nlBuffer, OVS_CT_LIMIT_ATTR_ZONE_LIMIT); + if (!offset) { + /* Starting the nested attribute failed. */ + status = STATUS_INVALID_BUFFER_SIZE; + goto done; + } + + /* Insert OVS_CT_ZONE_LIMIT attributes.*/ + for (UINT16 i = 0; i < numAttrs; i++) { + if (zoneLimitAttr) { + zoneLimitAttr->limit = zoneInfo[zoneLimitAttr->zone_id].limit; + zoneLimitAttr->count = zoneInfo[zoneLimitAttr->zone_id].entries; + if (zoneLimitAttr->zone_id == -1) { + zoneLimitAttr->limit = defaultCtLimit; + } + NlMsgPutTail(&nlBuffer, (const PCHAR)zoneLimitAttr, + sizeof(OVS_CT_ZONE_LIMIT)); + } else { + status = STATUS_INVALID_PARAMETER; + break; + } + zoneLimitAttr = (POVS_CT_ZONE_LIMIT)((PCHAR) zoneLimitAttr + + sizeof(OVS_CT_ZONE_LIMIT)); + } + NlMsgEndNested(&nlBuffer, offset); + } + +done: + nlMsg = (PNL_MSG_HDR)NlBufAt(&nlBuffer, 0, 0); + nlMsg->nlmsgLen = NlBufSize(&nlBuffer); + + return status; +} + +NTSTATUS +OvsCtLimitHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, + UINT32 *replyLen) +{ + NTSTATUS status; + POVS_MESSAGE msgIn = (POVS_MESSAGE)usrParamsCtx->inputBuffer; + POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer; + PNL_MSG_HDR nlMsgHdr = &(msgIn->nlMsg); + PGENL_MSG_HDR genlMsgHdr = &(msgIn->genlMsg); + POVS_HDR ovsHdr = &(msgIn->ovsHdr); + PCHAR attr = NULL; + UINT32 numAttrs = 0; + UINT32 attrOffset = NLMSG_HDRLEN + GENL_HDRLEN + OVS_HDRLEN; + + static const NL_POLICY ovsCtLimitPolicy[] = { + [OVS_CT_LIMIT_ATTR_ZONE_LIMIT] = { .type = NL_A_NESTED, .optional = TRUE } + }; + PNL_ATTR nlAttrs[ARRAY_SIZE(ovsCtLimitPolicy)]; + + if ((NlAttrParse(nlMsgHdr, attrOffset, NlMsgAttrsLen(nlMsgHdr), + ovsCtLimitPolicy, ARRAY_SIZE(ovsCtLimitPolicy), + nlAttrs, ARRAY_SIZE(nlAttrs))) + != TRUE) { + OVS_LOG_ERROR("Attr Parsing failed for msg: %p", nlMsgHdr); + return STATUS_INVALID_PARAMETER; + } + + if (nlAttrs[OVS_CT_LIMIT_ATTR_ZONE_LIMIT]) { + numAttrs = NlAttrGetSize(nlAttrs[OVS_CT_LIMIT_ATTR_ZONE_LIMIT])/sizeof(OVS_CT_ZONE_LIMIT); + attr = NlAttrGet(nlAttrs[OVS_CT_LIMIT_ATTR_ZONE_LIMIT]); + } + + if (genlMsgHdr->cmd == OVS_CT_LIMIT_CMD_SET || + genlMsgHdr->cmd == OVS_CT_LIMIT_CMD_DEL) { + POVS_CT_ZONE_LIMIT zoneLimitAttr = (POVS_CT_ZONE_LIMIT)attr; + for (UINT16 i = 0; i < numAttrs; i++) { + /* Parse zone limit attributes. */ + if (zoneLimitAttr) { + if (genlMsgHdr->cmd == OVS_CT_LIMIT_CMD_DEL) { + zoneLimitAttr->limit = CT_MAX_ENTRIES; + } + OvsCtSetZoneLimit(zoneLimitAttr->zone_id, zoneLimitAttr->limit); + } else { + OVS_LOG_ERROR("Failed to get zone limit attribute at index(%u)," + " numAttrs(%u)", i, numAttrs); + return STATUS_INVALID_PARAMETER; + } + zoneLimitAttr = (POVS_CT_ZONE_LIMIT)((PCHAR) zoneLimitAttr + + sizeof(OVS_CT_ZONE_LIMIT)); + } + } + + /* Output buffer has been validated while validating transact dev op. */ + ASSERT(msgOut != NULL && usrParamsCtx->outputLength >= sizeof *msgOut); + status = OvsCreateNlMsgFromCtLimit(msgIn, msgOut, + usrParamsCtx->outputLength, + attr, numAttrs, ovsHdr->dp_ifindex); + *replyLen = msgOut->nlMsg.nlmsgLen; + + return status; +} + #pragma warning(pop) diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h index d4152b3..78687dd 100644 --- a/datapath-windows/ovsext/Conntrack.h +++ b/datapath-windows/ovsext/Conntrack.h @@ -132,6 +132,18 @@ typedef struct OvsConntrackKeyLookupCtx { BOOLEAN related; } OvsConntrackKeyLookupCtx; +/* Per zone strucuture. */ +typedef struct _OVS_CT_ZONE_INFO { + ULONG limit; + ULONG entries; +} OVS_CT_ZONE_INFO, *POVS_CT_ZONE_INFO; + +typedef struct _OVS_CT_ZONE_LIMIT { + int zone_id; + UINT32 limit; + UINT32 count; +} OVS_CT_ZONE_LIMIT, *POVS_CT_ZONE_LIMIT; + #define CT_MAX_ENTRIES 1 << 21 #define CT_HASH_TABLE_SIZE ((UINT32)1 << 10) #define CT_HASH_TABLE_MASK (CT_HASH_TABLE_SIZE - 1) diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c index 34ef2b4..fa99484 100644 --- a/datapath-windows/ovsext/Datapath.c +++ b/datapath-windows/ovsext/Datapath.c @@ -99,7 +99,8 @@ NetlinkCmdHandler OvsGetNetdevCmdHandler, OvsSubscribePacketCmdHandler, OvsReadPacketCmdHandler, OvsCtDeleteCmdHandler, - OvsCtDumpCmdHandler; + OvsCtDumpCmdHandler, + OvsCtLimitHandler; static NTSTATUS HandleGetDpTransaction(POVS_USER_PARAMS_CONTEXT usrParamsCtx, UINT32 *replyLen); @@ -324,6 +325,34 @@ NETLINK_FAMILY nlNetdevFamilyOps = { .opsCount = ARRAY_SIZE(nlNetdevFamilyCmdOps) }; + +/* Netlink conntrack limit family. */ +NETLINK_CMD nlCtLimitFamilyCmdOps[] = { + { .cmd = OVS_CT_LIMIT_CMD_GET, + .handler = OvsCtLimitHandler, + .supportedDevOp = OVS_TRANSACTION_DEV_OP, + .validateDpIndex = FALSE + }, + { .cmd = OVS_CT_LIMIT_CMD_SET, + .handler = OvsCtLimitHandler, + .supportedDevOp = OVS_TRANSACTION_DEV_OP, + .validateDpIndex = FALSE + }, + { .cmd = OVS_CT_LIMIT_CMD_DEL, + .handler = OvsCtLimitHandler, + .supportedDevOp = OVS_TRANSACTION_DEV_OP, + .validateDpIndex = FALSE + }, +}; + +NETLINK_FAMILY nlCtLimitFamilyOps = { + .name = OVS_CT_LIMIT_FAMILY, + .id = OVS_WIN_NL_CTLIMIT_FAMILY_ID, + .version = OVS_CT_LIMIT_VERSION, + .maxAttr = OVS_CT_LIMIT_ATTR_MAX, + .cmds = nlCtLimitFamilyCmdOps, + .opsCount = ARRAY_SIZE(nlCtLimitFamilyCmdOps) +}; static NTSTATUS MapIrpOutputBuffer(PIRP irp, UINT32 bufferLength, UINT32 requiredLength, @@ -941,6 +970,9 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject, case OVS_WIN_NL_NETDEV_FAMILY_ID: nlFamilyOps = &nlNetdevFamilyOps; break; + case OVS_WIN_NL_CTLIMIT_FAMILY_ID: + nlFamilyOps = &nlCtLimitFamilyOps; + break; default: status = STATUS_INVALID_PARAMETER; goto done; diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c index 5234ca3..47077e9 100644 --- a/lib/netlink-socket.c +++ b/lib/netlink-socket.c @@ -1571,6 +1571,11 @@ do_lookup_genl_family(const char *name, struct nlattr **attrs, family_name = OVS_WIN_NETDEV_FAMILY; family_version = OVS_WIN_NETDEV_VERSION; family_attrmax = OVS_WIN_NETDEV_ATTR_MAX; + } else if (!strcmp(name, OVS_CT_LIMIT_FAMILY)) { + family_id = OVS_WIN_NL_CTLIMIT_FAMILY_ID; + family_name = OVS_CT_LIMIT_FAMILY; + family_version = OVS_CT_LIMIT_VERSION; + family_attrmax = OVS_CT_LIMIT_ATTR_MAX; } else { ofpbuf_delete(reply); return EINVAL; -- 2.9.3.windows.1 _______________________________________________ dev mailing list [email protected] https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Cvsairam%40vmware.com%7C1034a65e093e43e5a44f08d607b133cd%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636704855021298503&sdata=KO%2BoukTFo1vBd3b72%2FFiMuT636%2BAEsSrjH9wCCEYurE%3D&reserved=0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
