Re: [ovs-dev] [PATCH] datapath-windows: Update ICMP-Type and Code comparison in CT lookup

2017-08-15 Thread Anand Kumar 
Hi Alin,

Thanks for the review. Please find my responses inline.
Will address the review comments and send out v2 version of the patch.

Thanks,
Anand Kumar

On 8/14/17, 8:04 PM, "Alin Serdean"  wrote:

Hi Sai and Anand,

Thanks a lot for the patch. I have a few questions regarding the approach. 
Please see inline.

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Anand Kumar
> Sent: Friday, August 11, 2017 11:42 PM
> To: d...@openvswitch.org
    > Subject: [ovs-dev] [PATCH] datapath-windows: Update ICMP-Type and Code
    > comparison in CT lookup
> 
>  - Update the CT comparison function to compare individual fields instead 
of
> NdisEqualMemory.
[Alin Serdean] I don't like this change, especially mixing both members of 
union, i.e:
> +(ctxKey.dst.port == entryKey.dst.port) &&
> +(ctxKey.dst.icmp_id == entryKey.dst.icmp_id) &&
Why are you trying to change via member by member comparison?
   [Anand Kumar]: Done. Previously, we ran into issues while verifying NAT with 
ICMP, which now is fixed with my other patch.
> - Add in some padding for the ct_endpoint's union.
[Alin Serdean] Why is this needed? 
[Anand Kumar] This is needed because size of the union is 32 bits. 
   Another question do we still need the 'pad' member inside ct_endpoint 
(https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs_blob_master_datapath-2Dwindows_ovsext_Conntrack.h-23L51&d=DwIFAg&c=uilaK90D4TOVoH58JNXRgQ&r=Q5z9tBe-nAOpE7LIHSPV8uy5-437agMXvkeHHMkR8Us&m=1Rj8RlVaAovz2iLrWHvT6N469qFN036rzjLkBJweZDw&s=hky8AVI9m8GjZOD8r21WYvEHBPcDcVVZiwMKqk07IwU&e=
 ) ?
[Anand Kumar] Yes, the padding at the end of structure is no longer needed. 
I will address in next version.
> - Update the Orig Tuple to use ICMP Type and Code instead of Port for ICMP
[Alin Serdean] Agreed
> 
> Co-authored-by: Sairam Venugopal 
> Signed-off-by: Anand Kumar 






___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows: Update ICMP-Type and Code comparison in CT lookup

2017-08-14 Thread Alin Serdean 
Hi Sai and Anand,

Thanks a lot for the patch. I have a few questions regarding the approach. 
Please see inline.

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Anand Kumar
> Sent: Friday, August 11, 2017 11:42 PM
> To: d...@openvswitch.org
> Subject: [ovs-dev] [PATCH] datapath-windows: Update ICMP-Type and Code
> comparison in CT lookup
> 
>  - Update the CT comparison function to compare individual fields instead of
> NdisEqualMemory.
[Alin Serdean] I don't like this change, especially mixing both members of 
union, i.e:
> +(ctxKey.dst.port == entryKey.dst.port) &&
> +(ctxKey.dst.icmp_id == entryKey.dst.icmp_id) &&
Why are you trying to change via member by member comparison?
> - Add in some padding for the ct_endpoint's union.
[Alin Serdean] Why is this needed? Another question do we still need the 'pad' 
member inside ct_endpoint 
(https://github.com/openvswitch/ovs/blob/master/datapath-windows/ovsext/Conntrack.h#L51)
 ?
> - Update the Orig Tuple to use ICMP Type and Code instead of Port for ICMP
[Alin Serdean] Agreed
> 
> Co-authored-by: Sairam Venugopal 
> Signed-off-by: Anand Kumar 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows: Update ICMP-Type and Code comparison in CT lookup

2017-08-11 Thread Sairam Venugopal 
Acked-by: Sairam Venugopal 





On 8/11/17, 1:41 PM, "ovs-dev-boun...@openvswitch.org on behalf of Anand Kumar" 
 wrote:

> - Update the CT comparison function to compare individual fields instead of
>NdisEqualMemory.
>- Add in some padding for the ct_endpoint's union.
>- Update the Orig Tuple to use ICMP Type and Code instead of Port for ICMP
>
>Co-authored-by: Sairam Venugopal 
>Signed-off-by: Anand Kumar 
>---
> datapath-windows/ovsext/Conntrack.c | 27 +--
> datapath-windows/ovsext/Conntrack.h |  5 -
> 2 files changed, 25 insertions(+), 7 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Conntrack.c 
>b/datapath-windows/ovsext/Conntrack.c
>index 917ebee..81c2167 100644
>--- a/datapath-windows/ovsext/Conntrack.c
>+++ b/datapath-windows/ovsext/Conntrack.c
>@@ -373,10 +373,18 @@ OvsDetectCtPacket(OvsForwardingContext *fwdCtx,
> BOOLEAN
> OvsCtKeyAreSame(OVS_CT_KEY ctxKey, OVS_CT_KEY entryKey)
> {
>-return ((NdisEqualMemory(&ctxKey.src, &entryKey.src,
>- sizeof(struct ct_endpoint))) &&
>-(NdisEqualMemory(&ctxKey.dst, &entryKey.dst,
>- sizeof(struct ct_endpoint))) &&
>+return ((ctxKey.src.addr.ipv4 == entryKey.src.addr.ipv4) &&
>+(ctxKey.src.addr.ipv4_aligned == entryKey.src.addr.ipv4_aligned) 
>&&
>+(ctxKey.src.port == entryKey.src.port) &&
>+(ctxKey.src.icmp_id == entryKey.src.icmp_id) &&
>+(ctxKey.src.icmp_type == entryKey.src.icmp_type) &&
>+(ctxKey.src.icmp_code == entryKey.src.icmp_code) &&
>+(ctxKey.dst.addr.ipv4 == entryKey.dst.addr.ipv4) &&
>+(ctxKey.dst.addr.ipv4_aligned == entryKey.dst.addr.ipv4_aligned) 
>&&
>+(ctxKey.dst.port == entryKey.dst.port) &&
>+(ctxKey.dst.icmp_id == entryKey.dst.icmp_id) &&
>+(ctxKey.dst.icmp_type == entryKey.dst.icmp_type) &&
>+(ctxKey.dst.icmp_code == entryKey.dst.icmp_code) &&
> (ctxKey.dl_type == entryKey.dl_type) &&
> (ctxKey.nw_proto == entryKey.nw_proto) &&
> (ctxKey.zone == entryKey.zone));
>@@ -782,9 +790,16 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx,
> 
> key->ct.tuple_ipv4.ipv4_src = ctKey->src.addr.ipv4_aligned;
> key->ct.tuple_ipv4.ipv4_dst = ctKey->dst.addr.ipv4_aligned;
>-key->ct.tuple_ipv4.src_port = ctKey->src.port;
>-key->ct.tuple_ipv4.dst_port = ctKey->dst.port;
> key->ct.tuple_ipv4.ipv4_proto = ctKey->nw_proto;
>+
>+/* Orig tuple Port is overloaded to take in ICMP-Type & Code */
>+/* This mimics the behavior in lib/conntrack.c*/
>+key->ct.tuple_ipv4.src_port = ctKey->nw_proto != IPPROTO_ICMP ?
>+  ctKey->src.port :
>+  htons(ctKey->src.icmp_type);
>+key->ct.tuple_ipv4.dst_port = ctKey->nw_proto != IPPROTO_ICMP ?
>+  ctKey->dst.port :
>+  htons(ctKey->src.icmp_code);
> }
> 
> if (entryCreated && entry) {
>diff --git a/datapath-windows/ovsext/Conntrack.h 
>b/datapath-windows/ovsext/Conntrack.h
>index 04ca299..4904c7e 100644
>--- a/datapath-windows/ovsext/Conntrack.h
>+++ b/datapath-windows/ovsext/Conntrack.h
>@@ -41,7 +41,10 @@ struct ct_addr {
> struct ct_endpoint {
> struct ct_addr addr;
> union {
>-ovs_be16 port;
>+struct {
>+ovs_be16 port;
>+uint16 pad_port;
>+};
> struct {
> ovs_be16 icmp_id;
> uint8_t icmp_type;
>-- 
>2.9.3.windows.1
>
>___
>dev mailing list
>d...@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=jv5eh3bLA-gmtkDuLhimROtMSTOYl817LE81oKgfKo8&s=zIJGnXCflvEaG9FWrbvkouqo9uSfZ-KoRq7H_GzPiMo&e=
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] datapath-windows: Update ICMP-Type and Code comparison in CT lookup

2017-08-11 Thread Anand Kumar 
 - Update the CT comparison function to compare individual fields instead of
NdisEqualMemory.
- Add in some padding for the ct_endpoint's union.
- Update the Orig Tuple to use ICMP Type and Code instead of Port for ICMP

Co-authored-by: Sairam Venugopal 
Signed-off-by: Anand Kumar 
---
 datapath-windows/ovsext/Conntrack.c | 27 +--
 datapath-windows/ovsext/Conntrack.h |  5 -
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index 917ebee..81c2167 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -373,10 +373,18 @@ OvsDetectCtPacket(OvsForwardingContext *fwdCtx,
 BOOLEAN
 OvsCtKeyAreSame(OVS_CT_KEY ctxKey, OVS_CT_KEY entryKey)
 {
-return ((NdisEqualMemory(&ctxKey.src, &entryKey.src,
- sizeof(struct ct_endpoint))) &&
-(NdisEqualMemory(&ctxKey.dst, &entryKey.dst,
- sizeof(struct ct_endpoint))) &&
+return ((ctxKey.src.addr.ipv4 == entryKey.src.addr.ipv4) &&
+(ctxKey.src.addr.ipv4_aligned == entryKey.src.addr.ipv4_aligned) &&
+(ctxKey.src.port == entryKey.src.port) &&
+(ctxKey.src.icmp_id == entryKey.src.icmp_id) &&
+(ctxKey.src.icmp_type == entryKey.src.icmp_type) &&
+(ctxKey.src.icmp_code == entryKey.src.icmp_code) &&
+(ctxKey.dst.addr.ipv4 == entryKey.dst.addr.ipv4) &&
+(ctxKey.dst.addr.ipv4_aligned == entryKey.dst.addr.ipv4_aligned) &&
+(ctxKey.dst.port == entryKey.dst.port) &&
+(ctxKey.dst.icmp_id == entryKey.dst.icmp_id) &&
+(ctxKey.dst.icmp_type == entryKey.dst.icmp_type) &&
+(ctxKey.dst.icmp_code == entryKey.dst.icmp_code) &&
 (ctxKey.dl_type == entryKey.dl_type) &&
 (ctxKey.nw_proto == entryKey.nw_proto) &&
 (ctxKey.zone == entryKey.zone));
@@ -782,9 +790,16 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx,
 
 key->ct.tuple_ipv4.ipv4_src = ctKey->src.addr.ipv4_aligned;
 key->ct.tuple_ipv4.ipv4_dst = ctKey->dst.addr.ipv4_aligned;
-key->ct.tuple_ipv4.src_port = ctKey->src.port;
-key->ct.tuple_ipv4.dst_port = ctKey->dst.port;
 key->ct.tuple_ipv4.ipv4_proto = ctKey->nw_proto;
+
+/* Orig tuple Port is overloaded to take in ICMP-Type & Code */
+/* This mimics the behavior in lib/conntrack.c*/
+key->ct.tuple_ipv4.src_port = ctKey->nw_proto != IPPROTO_ICMP ?
+  ctKey->src.port :
+  htons(ctKey->src.icmp_type);
+key->ct.tuple_ipv4.dst_port = ctKey->nw_proto != IPPROTO_ICMP ?
+  ctKey->dst.port :
+  htons(ctKey->src.icmp_code);
 }
 
 if (entryCreated && entry) {
diff --git a/datapath-windows/ovsext/Conntrack.h 
b/datapath-windows/ovsext/Conntrack.h
index 04ca299..4904c7e 100644
--- a/datapath-windows/ovsext/Conntrack.h
+++ b/datapath-windows/ovsext/Conntrack.h
@@ -41,7 +41,10 @@ struct ct_addr {
 struct ct_endpoint {
 struct ct_addr addr;
 union {
-ovs_be16 port;
+struct {
+ovs_be16 port;
+uint16 pad_port;
+};
 struct {
 ovs_be16 icmp_id;
 uint8_t icmp_type;
-- 
2.9.3.windows.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev