Re: [ovs-dev] [PATCH v4] ofproto-dpif-xlate: Remove repeated function for judge garp
On Mon, Feb 20, 2023 at 5:05 AM Han Ding wrote:
>
>
> Function is_gratuitous_arp() and function is_garp() are all used to judge
> whether the flow is gratuitous arp. It is not necessary to use two functions
> to do the same thing and just keep one.
>
> Gratuitous ARP message is a generally link-level broadcast messages and
> carry the same IP in sender and target fields. This patch add the check
> in function is_garp() whether the arp is a broadcast message and whether
> the arp is an arp request or an arp reply.
>
> Signed-off-by: Han Ding
> ---
>
> Notes:
> v2:
> - Add the check whether the ARP packet is a broadcast message in
> is_garp().
>
> v3:
> - Add the check whether the packet is an arp request or an arp reply in
> is_garp().
>
> v4:
> - Add description of differences between patch versions
> - Use WC_MASK_FIELD() macro instead of memset.
>
> lib/flow.h | 16 +---
> ofproto/ofproto-dpif-xlate.c | 32 ++--
> 2 files changed, 15 insertions(+), 33 deletions(-)
>
> diff --git a/lib/flow.h b/lib/flow.h
> index c647ad83c..037deb6cd 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -1133,10 +1133,20 @@ static inline bool is_garp(const struct flow *flow,
> struct flow_wildcards *wc)
> {
> if (is_arp(flow)) {
> -return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
> -FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
> -}
> +if (!eth_addr_is_broadcast(FLOW_WC_GET_AND_MASK_WC(flow, wc,
> dl_dst))) {
This line is too long.
Other than that, This patch looks good to me. The new behaviour seems
to be more correct than before as well.
Thanks,
M
> +return false;
> +}
>
> +if (wc) {
> +WC_MASK_FIELD(wc, nw_proto);
> +}
> +
> +if (flow->nw_proto == ARP_OP_REQUEST ||
> +flow->nw_proto == ARP_OP_REPLY) {
> +return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
> +FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
> +}
> +}
> return false;
> }
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index a9cf3cbee..b3c13f6bf 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct
> xbundle *out_xbundle,
> memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
> }
>
> -/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
> - * migration. Older Citrix-patched Linux DomU used gratuitous ARP replies to
> - * indicate this; newer upstream kernels use gratuitous ARP requests. */
> -static bool
> -is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc)
> -{
> -if (flow->dl_type != htons(ETH_TYPE_ARP)) {
> -return false;
> -}
> -
> -memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
> -if (!eth_addr_is_broadcast(flow->dl_dst)) {
> -return false;
> -}
> -
> -memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
> -if (flow->nw_proto == ARP_OP_REPLY) {
> -return true;
> -} else if (flow->nw_proto == ARP_OP_REQUEST) {
> -memset(>masks.nw_src, 0xff, sizeof wc->masks.nw_src);
> -memset(>masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
> -
> -return flow->nw_src == flow->nw_dst;
> -} else {
> -return false;
> -}
> -}
> -
> /* Determines whether packets in 'flow' within 'xbridge' should be forwarded
> or
> * dropped. Returns true if they may be forwarded, false if they should be
> * dropped.
> @@ -2619,7 +2591,7 @@ is_admissible(struct xlate_ctx *ctx, struct xport
> *in_port,
> mac = mac_learning_lookup(xbridge->ml, flow->dl_src, vlan);
> if (mac
> && mac_entry_get_port(xbridge->ml, mac) !=
> in_xbundle->ofbundle
> -&& (!is_gratuitous_arp(flow, ctx->wc)
> +&& (!is_garp(flow, ctx->wc)
> || mac_entry_is_grat_arp_locked(mac))) {
> ovs_rwlock_unlock(>ml->rwlock);
> xlate_report(ctx, OFT_DETAIL,
> @@ -3062,7 +3034,7 @@ xlate_normal(struct xlate_ctx *ctx)
> }
>
> /* Learn source MAC. */
> -bool is_grat_arp = is_gratuitous_arp(flow, wc);
> +bool is_grat_arp = is_garp(flow, wc);
> if (ctx->xin->allow_side_effects
> && flow->packet_type == htonl(PT_ETH)
> && in_port && in_port->pt_mode != NETDEV_PT_LEGACY_L3
> --
> 2.27.0
>
>
>
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] ofproto-dpif-xlate: Remove repeated function for judge garp
Bleep bloop. Greetings Han Ding, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 80 characters long (recommended limit is 79)
#32 FILE: lib/flow.h:1136:
if (!eth_addr_is_broadcast(FLOW_WC_GET_AND_MASK_WC(flow, wc, dl_dst))) {
Lines checked: 109, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email
acon...@redhat.com
Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4] ofproto-dpif-xlate: Remove repeated function for judge garp
Function is_gratuitous_arp() and function is_garp() are all used to judge
whether the flow is gratuitous arp. It is not necessary to use two functions
to do the same thing and just keep one.
Gratuitous ARP message is a generally link-level broadcast messages and
carry the same IP in sender and target fields. This patch add the check
in function is_garp() whether the arp is a broadcast message and whether
the arp is an arp request or an arp reply.
Signed-off-by: Han Ding
---
Notes:
v2:
- Add the check whether the ARP packet is a broadcast message in is_garp().
v3:
- Add the check whether the packet is an arp request or an arp reply in
is_garp().
v4:
- Add description of differences between patch versions
- Use WC_MASK_FIELD() macro instead of memset.
lib/flow.h | 16 +---
ofproto/ofproto-dpif-xlate.c | 32 ++--
2 files changed, 15 insertions(+), 33 deletions(-)
diff --git a/lib/flow.h b/lib/flow.h
index c647ad83c..037deb6cd 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -1133,10 +1133,20 @@ static inline bool is_garp(const struct flow *flow,
struct flow_wildcards *wc)
{
if (is_arp(flow)) {
-return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
-FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
-}
+if (!eth_addr_is_broadcast(FLOW_WC_GET_AND_MASK_WC(flow, wc, dl_dst)))
{
+return false;
+}
+if (wc) {
+WC_MASK_FIELD(wc, nw_proto);
+}
+
+if (flow->nw_proto == ARP_OP_REQUEST ||
+flow->nw_proto == ARP_OP_REPLY) {
+return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
+FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
+}
+}
return false;
}
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a9cf3cbee..b3c13f6bf 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct
xbundle *out_xbundle,
memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
}
-/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
- * migration. Older Citrix-patched Linux DomU used gratuitous ARP replies to
- * indicate this; newer upstream kernels use gratuitous ARP requests. */
-static bool
-is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc)
-{
-if (flow->dl_type != htons(ETH_TYPE_ARP)) {
-return false;
-}
-
-memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
-if (!eth_addr_is_broadcast(flow->dl_dst)) {
-return false;
-}
-
-memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
-if (flow->nw_proto == ARP_OP_REPLY) {
-return true;
-} else if (flow->nw_proto == ARP_OP_REQUEST) {
-memset(>masks.nw_src, 0xff, sizeof wc->masks.nw_src);
-memset(>masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
-
-return flow->nw_src == flow->nw_dst;
-} else {
-return false;
-}
-}
-
/* Determines whether packets in 'flow' within 'xbridge' should be forwarded or
* dropped. Returns true if they may be forwarded, false if they should be
* dropped.
@@ -2619,7 +2591,7 @@ is_admissible(struct xlate_ctx *ctx, struct xport
*in_port,
mac = mac_learning_lookup(xbridge->ml, flow->dl_src, vlan);
if (mac
&& mac_entry_get_port(xbridge->ml, mac) != in_xbundle->ofbundle
-&& (!is_gratuitous_arp(flow, ctx->wc)
+&& (!is_garp(flow, ctx->wc)
|| mac_entry_is_grat_arp_locked(mac))) {
ovs_rwlock_unlock(>ml->rwlock);
xlate_report(ctx, OFT_DETAIL,
@@ -3062,7 +3034,7 @@ xlate_normal(struct xlate_ctx *ctx)
}
/* Learn source MAC. */
-bool is_grat_arp = is_gratuitous_arp(flow, wc);
+bool is_grat_arp = is_garp(flow, wc);
if (ctx->xin->allow_side_effects
&& flow->packet_type == htonl(PT_ETH)
&& in_port && in_port->pt_mode != NETDEV_PT_LEGACY_L3
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4] ofproto-dpif-xlate: Remove repeated function for judge garp
Function is_gratuitous_arp() and function is_garp() are all used to judge
whether the flow is gratuitous arp. It is not necessary to use two functions
to do the same thing and just keep one.
Gratuitous ARP message is a generally link-level broadcast messages and
carry the same IP in sender and target fields. This patch add the check
in function is_garp() whether the arp is a broadcast message and whether
the arp is an arp request or an arp reply.
Signed-off-by: Han Ding
---
Notes:
v2:
- Add the check whether the ARP packet is a broadcast message in is_garp().
v3:
- Add the check whether the packet is an arp request or an arp reply in
is_garp().
v4:
- Add description of differences between patch versions
- Use WC_MASK_FIELD() macro instead of memset.
lib/flow.h | 19 +--
ofproto/ofproto-dpif-xlate.c | 32 ++--
2 files changed, 19 insertions(+), 32 deletions(-)
diff --git a/lib/flow.h b/lib/flow.h
index c647ad83c..9294a1b24 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -1133,8 +1133,23 @@ static inline bool is_garp(const struct flow *flow,
struct flow_wildcards *wc)
{
if (is_arp(flow)) {
-return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
-FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
+if (wc) {
+WC_MASK_FIELD(wc, dl_dst);
+}
+
+if (!eth_addr_is_broadcast(flow->dl_dst)) {
+return false;
+}
+
+if (wc) {
+WC_MASK_FIELD(wc, nw_proto);
+}
+
+if (flow->nw_proto == ARP_OP_REQUEST ||
+flow->nw_proto == ARP_OP_REPLY) {
+return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
+FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
+}
}
return false;
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a9cf3cbee..b3c13f6bf 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct
xbundle *out_xbundle,
memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
}
-/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
- * migration. Older Citrix-patched Linux DomU used gratuitous ARP replies to
- * indicate this; newer upstream kernels use gratuitous ARP requests. */
-static bool
-is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc)
-{
-if (flow->dl_type != htons(ETH_TYPE_ARP)) {
-return false;
-}
-
-memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
-if (!eth_addr_is_broadcast(flow->dl_dst)) {
-return false;
-}
-
-memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
-if (flow->nw_proto == ARP_OP_REPLY) {
-return true;
-} else if (flow->nw_proto == ARP_OP_REQUEST) {
-memset(>masks.nw_src, 0xff, sizeof wc->masks.nw_src);
-memset(>masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
-
-return flow->nw_src == flow->nw_dst;
-} else {
-return false;
-}
-}
-
/* Determines whether packets in 'flow' within 'xbridge' should be forwarded or
* dropped. Returns true if they may be forwarded, false if they should be
* dropped.
@@ -2619,7 +2591,7 @@ is_admissible(struct xlate_ctx *ctx, struct xport
*in_port,
mac = mac_learning_lookup(xbridge->ml, flow->dl_src, vlan);
if (mac
&& mac_entry_get_port(xbridge->ml, mac) != in_xbundle->ofbundle
-&& (!is_gratuitous_arp(flow, ctx->wc)
+&& (!is_garp(flow, ctx->wc)
|| mac_entry_is_grat_arp_locked(mac))) {
ovs_rwlock_unlock(>ml->rwlock);
xlate_report(ctx, OFT_DETAIL,
@@ -3062,7 +3034,7 @@ xlate_normal(struct xlate_ctx *ctx)
}
/* Learn source MAC. */
-bool is_grat_arp = is_gratuitous_arp(flow, wc);
+bool is_grat_arp = is_garp(flow, wc);
if (ctx->xin->allow_side_effects
&& flow->packet_type == htonl(PT_ETH)
&& in_port && in_port->pt_mode != NETDEV_PT_LEGACY_L3
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
