| Message ID | 20170905235304.4016-1-kumaranand@vmware.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] datapath-windows: Increment ct packet counters based on ct_state. | expand |
Comments inline.
I guess the Conntrack code is intentionally styled similar to the user space code with the idea of sharing it. I am not sure if this is realistic or even possible now, given that the entire Conntrack code is re-written for the Windows data path. If there is not going to be any code sharing, it does not make sense to style the CT code differently than the rest of the Windows data path code.
---
Acked-by: Shashank Ram
Acked-by: Sairam Venugopal <vsairam@vmware.com> On 9/5/17, 4:53 PM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote: >For a given packet, packet counters in conntrack should be accounted only >once, even if the packet is processed multiple times by conntrack. > >When a packet is processed by conntrack, ct_state flag is set to >OVS_CS_F_TRACKED. Use this state to identify if a packet has been >processed previously by conntrack. > >Also update the ct packet counters when ct entry is created. > >With this patch, the conntrack's packet counters behavior is similar >to linux > >Signed-off-by: Anand Kumar <kumaranand@vmware.com> >--- > datapath-windows/ovsext/Conntrack.c | 34 +++++++++++++++++++++------------- > 1 file changed, 21 insertions(+), 13 deletions(-) > >diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c >index 8bcda05..0adb6d5 100644 >--- a/datapath-windows/ovsext/Conntrack.c >+++ b/datapath-windows/ovsext/Conntrack.c >@@ -169,6 +169,18 @@ OvsPostCtEventEntry(POVS_CT_ENTRY entry, UINT8 type) > OvsPostCtEvent(&ctEventEntry); > } > >+static __inline VOID >+OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN reply, PNET_BUFFER_LIST nbl) >+{ >+ if (reply) { >+ entry->rev_key.byteCount+= OvsPacketLenNBL(nbl); >+ entry->rev_key.packetCount++; >+ } else { >+ entry->key.byteCount += OvsPacketLenNBL(nbl); >+ entry->key.packetCount++; >+ } >+} >+ > static __inline BOOLEAN > OvsCtAddEntry(POVS_CT_ENTRY entry, OvsConntrackKeyLookupCtx *ctx, > PNAT_ACTION_INFO natInfo, UINT64 now) >@@ -287,6 +299,9 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx, > } > > OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL); >+ if (entry) { >+ OvsCtIncrementCounters(entry, ctx->reply, curNbl); >+ } > return entry; > } > >@@ -382,18 +397,6 @@ OvsCtKeyAreSame(OVS_CT_KEY ctxKey, OVS_CT_KEY entryKey) > (ctxKey.zone == entryKey.zone)); > } > >-static __inline VOID >-OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN reply, PNET_BUFFER_LIST nbl) >-{ >- if (reply) { >- entry->rev_key.byteCount+= OvsPacketLenNBL(nbl); >- entry->rev_key.packetCount++; >- } else { >- entry->key.byteCount += OvsPacketLenNBL(nbl); >- entry->key.packetCount++; >- } >-} >- > POVS_CT_ENTRY > OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) > { >@@ -730,6 +733,12 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, > NdisReleaseRWLock(ovsConntrackLockObj, &lockState); > OVS_LOG_ERROR("Conntrack Limit hit: %lu", ctTotalEntries); > return NDIS_STATUS_RESOURCES; >+ >+ /* Increment the counters soon after the lookup, since we set ct.state >+ * to OVS_CS_F_TRACKED after processing the ct entry. >+ */ >+ if (entry && (!(key->ct.state & OVS_CS_F_TRACKED))) { >+ OvsCtIncrementCounters(entry, ctx.reply, curNbl); > } > > if (!entry) { >@@ -740,7 +749,6 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, > &entryCreated); > } else { > /* Process the entry and update CT flags */ >- OvsCtIncrementCounters(entry, ctx.reply, curNbl); > entry = OvsProcessConntrackEntry(fwdCtx, layers->l4Offset, &ctx, key, > zone, natInfo, commit, currentTime, > &entryCreated); >-- >2.9.3.windows.1 > >_______________________________________________ >dev mailing list >dev@openvswitch.org >https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=UUkY3ld1vxNjRMbNPm79bHFxEAWK0F9-VhuWaYlU0KM&s=W9Yf7wNSiWN-x1a9C_wSYqDGpCswFcmcyoNzE8rpuwk&e=
Acked-by: Alin Serdean <aserdean@ovn.org> > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Anand Kumar > Sent: Wednesday, September 6, 2017 2:53 AM > To: dev@openvswitch.org > Subject: [ovs-dev] [PATCH] datapath-windows: Increment ct packet counters > based on ct_state. > > For a given packet, packet counters in conntrack should be accounted only > once, even if the packet is processed multiple times by conntrack. > > When a packet is processed by conntrack, ct_state flag is set to > OVS_CS_F_TRACKED. Use this state to identify if a packet has been > processed previously by conntrack. > > Also update the ct packet counters when ct entry is created. > > With this patch, the conntrack's packet counters behavior is similar to linux > > Signed-off-by: Anand Kumar <kumaranand@vmware.com> > --- > datapath-windows/ovsext/Conntrack.c | 34 +++++++++++++++++++++----- > -------- > 1 file changed, 21 insertions(+), 13 deletions(-) > > diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath- > windows/ovsext/Conntrack.c > index 8bcda05..0adb6d5 100644 > --- a/datapath-windows/ovsext/Conntrack.c > +++ b/datapath-windows/ovsext/Conntrack.c > @@ -169,6 +169,18 @@ OvsPostCtEventEntry(POVS_CT_ENTRY entry, UINT8 > type) > OvsPostCtEvent(&ctEventEntry); > } > > +static __inline VOID > +OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN reply, > +PNET_BUFFER_LIST nbl) { > + if (reply) { > + entry->rev_key.byteCount+= OvsPacketLenNBL(nbl); > + entry->rev_key.packetCount++; > + } else { > + entry->key.byteCount += OvsPacketLenNBL(nbl); > + entry->key.packetCount++; > + } > +} > + > static __inline BOOLEAN > OvsCtAddEntry(POVS_CT_ENTRY entry, OvsConntrackKeyLookupCtx *ctx, > PNAT_ACTION_INFO natInfo, UINT64 now) @@ -287,6 +299,9 @@ > OvsCtEntryCreate(OvsForwardingContext *fwdCtx, > } > > OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL); > + if (entry) { > + OvsCtIncrementCounters(entry, ctx->reply, curNbl); > + } > return entry; > } > > @@ -382,18 +397,6 @@ OvsCtKeyAreSame(OVS_CT_KEY ctxKey, > OVS_CT_KEY entryKey) > (ctxKey.zone == entryKey.zone)); } > > -static __inline VOID > -OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN reply, > PNET_BUFFER_LIST nbl) -{ > - if (reply) { > - entry->rev_key.byteCount+= OvsPacketLenNBL(nbl); > - entry->rev_key.packetCount++; > - } else { > - entry->key.byteCount += OvsPacketLenNBL(nbl); > - entry->key.packetCount++; > - } > -} > - > POVS_CT_ENTRY > OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) { @@ -730,6 +733,12 @@ > OvsCtExecute_(OvsForwardingContext *fwdCtx, > NdisReleaseRWLock(ovsConntrackLockObj, &lockState); > OVS_LOG_ERROR("Conntrack Limit hit: %lu", ctTotalEntries); > return NDIS_STATUS_RESOURCES; > + > + /* Increment the counters soon after the lookup, since we set ct.state > + * to OVS_CS_F_TRACKED after processing the ct entry. > + */ > + if (entry && (!(key->ct.state & OVS_CS_F_TRACKED))) { > + OvsCtIncrementCounters(entry, ctx.reply, curNbl); > } > > if (!entry) { > @@ -740,7 +749,6 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, > &entryCreated); > } else { > /* Process the entry and update CT flags */ > - OvsCtIncrementCounters(entry, ctx.reply, curNbl); > entry = OvsProcessConntrackEntry(fwdCtx, layers->l4Offset, &ctx, key, > zone, natInfo, commit, currentTime, > &entryCreated); > -- > 2.9.3.windows.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
I added a missing curly bracket to make it compile. Thanks Anand I applied it on master and branch-2.8. Alin. > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of aserdean@ovn.org > Sent: Thursday, September 7, 2017 9:54 PM > To: 'Anand Kumar' <kumaranand@vmware.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH] datapath-windows: Increment ct packet > counters based on ct_state. > > Acked-by: Alin Serdean <aserdean@ovn.org> > > > -----Original Message----- > > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > > bounces@openvswitch.org] On Behalf Of Anand Kumar > > Sent: Wednesday, September 6, 2017 2:53 AM > > To: dev@openvswitch.org > > Subject: [ovs-dev] [PATCH] datapath-windows: Increment ct packet > > counters based on ct_state. > > > > For a given packet, packet counters in conntrack should be accounted > > only once, even if the packet is processed multiple times by conntrack. > > > > When a packet is processed by conntrack, ct_state flag is set to > > OVS_CS_F_TRACKED. Use this state to identify if a packet has been > > processed previously by conntrack. > > > > Also update the ct packet counters when ct entry is created. > > > > With this patch, the conntrack's packet counters behavior is similar > > to > linux > > > > Signed-off-by: Anand Kumar <kumaranand@vmware.com> > > --- > > datapath-windows/ovsext/Conntrack.c | 34 +++++++++++++++++++++--- > -- > > -------- > > 1 file changed, 21 insertions(+), 13 deletions(-) > > > > diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath- > > windows/ovsext/Conntrack.c index 8bcda05..0adb6d5 100644 > > --- a/datapath-windows/ovsext/Conntrack.c > > +++ b/datapath-windows/ovsext/Conntrack.c > > @@ -169,6 +169,18 @@ OvsPostCtEventEntry(POVS_CT_ENTRY entry, > UINT8 > > type) > > OvsPostCtEvent(&ctEventEntry); > > } > > > > +static __inline VOID > > +OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN reply, > > +PNET_BUFFER_LIST nbl) { > > + if (reply) { > > + entry->rev_key.byteCount+= OvsPacketLenNBL(nbl); > > + entry->rev_key.packetCount++; > > + } else { > > + entry->key.byteCount += OvsPacketLenNBL(nbl); > > + entry->key.packetCount++; > > + } > > +} > > + > > static __inline BOOLEAN > > OvsCtAddEntry(POVS_CT_ENTRY entry, OvsConntrackKeyLookupCtx *ctx, > > PNAT_ACTION_INFO natInfo, UINT64 now) @@ -287,6 +299,9 > > @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx, > > } > > > > OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL); > > + if (entry) { > > + OvsCtIncrementCounters(entry, ctx->reply, curNbl); > > + } > > return entry; > > } > > > > @@ -382,18 +397,6 @@ OvsCtKeyAreSame(OVS_CT_KEY ctxKey, > OVS_CT_KEY > > entryKey) > > (ctxKey.zone == entryKey.zone)); } > > > > -static __inline VOID > > -OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN reply, > > PNET_BUFFER_LIST nbl) -{ > > - if (reply) { > > - entry->rev_key.byteCount+= OvsPacketLenNBL(nbl); > > - entry->rev_key.packetCount++; > > - } else { > > - entry->key.byteCount += OvsPacketLenNBL(nbl); > > - entry->key.packetCount++; > > - } > > -} > > - > > POVS_CT_ENTRY > > OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) { @@ -730,6 +733,12 @@ > > OvsCtExecute_(OvsForwardingContext *fwdCtx, > > NdisReleaseRWLock(ovsConntrackLockObj, &lockState); > > OVS_LOG_ERROR("Conntrack Limit hit: %lu", ctTotalEntries); > > return NDIS_STATUS_RESOURCES; > > + > > + /* Increment the counters soon after the lookup, since we set > ct.state > > + * to OVS_CS_F_TRACKED after processing the ct entry. > > + */ > > + if (entry && (!(key->ct.state & OVS_CS_F_TRACKED))) { > > + OvsCtIncrementCounters(entry, ctx.reply, curNbl); > > } > > > > if (!entry) { > > @@ -740,7 +749,6 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, > > &entryCreated); > > } else { > > /* Process the entry and update CT flags */ > > - OvsCtIncrementCounters(entry, ctx.reply, curNbl); > > entry = OvsProcessConntrackEntry(fwdCtx, layers->l4Offset, > > &ctx, > key, > > zone, natInfo, commit, > currentTime, > > &entryCreated); > > -- > > 2.9.3.windows.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c index 8bcda05..0adb6d5 100644 --- a/datapath-windows/ovsext/Conntrack.c +++ b/datapath-windows/ovsext/Conntrack.c @@ -169,6 +169,18 @@ OvsPostCtEventEntry(POVS_CT_ENTRY entry, UINT8 type) OvsPostCtEvent(&ctEventEntry); } +static __inline VOID +OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN reply, PNET_BUFFER_LIST nbl) +{ + if (reply) { + entry->rev_key.byteCount+= OvsPacketLenNBL(nbl); + entry->rev_key.packetCount++; + } else { + entry->key.byteCount += OvsPacketLenNBL(nbl); + entry->key.packetCount++; + } +} + static __inline BOOLEAN OvsCtAddEntry(POVS_CT_ENTRY entry, OvsConntrackKeyLookupCtx *ctx, PNAT_ACTION_INFO natInfo, UINT64 now) @@ -287,6 +299,9 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx, } OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL); + if (entry) { + OvsCtIncrementCounters(entry, ctx->reply, curNbl); + } return entry; } @@ -382,18 +397,6 @@ OvsCtKeyAreSame(OVS_CT_KEY ctxKey, OVS_CT_KEY entryKey) (ctxKey.zone == entryKey.zone)); } -static __inline VOID -OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN reply, PNET_BUFFER_LIST nbl) -{ - if (reply) { - entry->rev_key.byteCount+= OvsPacketLenNBL(nbl); - entry->rev_key.packetCount++; - } else { - entry->key.byteCount += OvsPacketLenNBL(nbl); - entry->key.packetCount++; - } -} - POVS_CT_ENTRY OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) { @@ -730,6 +733,12 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, NdisReleaseRWLock(ovsConntrackLockObj, &lockState); OVS_LOG_ERROR("Conntrack Limit hit: %lu", ctTotalEntries); return NDIS_STATUS_RESOURCES; + + /* Increment the counters soon after the lookup, since we set ct.state + * to OVS_CS_F_TRACKED after processing the ct entry. + */ + if (entry && (!(key->ct.state & OVS_CS_F_TRACKED))) { + OvsCtIncrementCounters(entry, ctx.reply, curNbl); } if (!entry) { @@ -740,7 +749,6 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, &entryCreated); } else { /* Process the entry and update CT flags */ - OvsCtIncrementCounters(entry, ctx.reply, curNbl); entry = OvsProcessConntrackEntry(fwdCtx, layers->l4Offset, &ctx, key, zone, natInfo, commit, currentTime, &entryCreated);
For a given packet, packet counters in conntrack should be accounted only once, even if the packet is processed multiple times by conntrack. When a packet is processed by conntrack, ct_state flag is set to OVS_CS_F_TRACKED. Use this state to identify if a packet has been processed previously by conntrack. Also update the ct packet counters when ct entry is created. With this patch, the conntrack's packet counters behavior is similar to linux Signed-off-by: Anand Kumar <kumaranand@vmware.com> --- datapath-windows/ovsext/Conntrack.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-)