[ovs-dev] [PATCH] datapath-windows: Add support to configure ct zone limits
Anand Kumar
kumaranand at vmware.com
Mon Aug 27 21:53:49 UTC 2018
Hi Shashank,
Thanks for reviewing the patch. Please find response inline.
Regards,
Anand Kumar
On 8/27/18, 9:00 AM, "Shashank Ram" <rams at vmware.com> wrote:
On 08/21/2018 02:57 PM, Anand Kumar 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 <kumaranand at vmware.com>
> ---
> 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();
> }
> +
> + zoneInfo = OvsAllocateMemoryWithTag(sizeof(OVS_CT_ZONE_INFO) *
> + (UINT16_MAX + 1), OVS_CT_POOL_TAG);
Please define UINT16_MAX as an appropriate macro in the CT module and
use that. It is not only more intuitive in the code, but is also more
safe in terms of the abstraction.
[AK]: Done
> + if (zoneInfo == NULL) {
> + status = STATUS_INSUFFICIENT_RESOURCES;
> + goto freeBucketLock;
> + }
> +
> + defaultCtLimit = CT_MAX_ENTRIES;
> + for (int i = 0; i <= UINT16_MAX; i++) {
Please define UINT16_MAX as CT_XXX
[AK]: Done
> + 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
> +OvsCtSetZoneLimit(int zone, ULONG value) {
> + if (zone == -1) {
> + /* Set default limit for all zones. */
> + defaultCtLimit = value;
> + for (UINT32 i = 0; i <= UINT16_MAX; i++) {
Why is a UINT32 being used as the index variable? This is because of not
properly making use of a well defined macro in the CT module that
abstracts UNINT16_MAX + 1 as the max limit.
[AK]: Need a 32 bit variable as the index, since max value exceeds 16 bits.
> + 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,
Move arguments to a separate line.
[AK]: Done
> + 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
Please use NDIS_STATUS
[AK]: The netlink handler takes NTSTATUS as return type. Will keep this as is, don’t want to change it.
> +OvsCreateNlMsgFromCtLimit(POVS_MESSAGE msgIn,
> + PVOID outBuffer,
> + UINT32 outBufLen,
> + PCHAR attr,
> + UINT32 numAttrs,
> + int dpIfIndex)
> +{
> + NTSTATUS status = STATUS_SUCCESS;
> + NL_BUFFER nlBuffer;
> + BOOLEAN ok;
Please use a better variable name.
[AK]: Not needed, will get rid of this.
> + 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
NDIS_STATUS
[AK]: The netlink handler expects this as return type, Will keep this as is, don’t want to change it.
> +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;
Why is the limit a ULONG in one place and UINT32 in the other? Please
make it consistent if necessary.
[AK]: Will change it to ULONG.
> +
> #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;
Thanks for the patch, please do fix the issues highlighted.
- Shashank
More information about the dev
mailing list