diff mbox series

[ovs-dev,ovn,6/7] ovn-northd.c: Remove the use of the REGBIT_SKIP_LOOKUP_NEIGHBOR bit.

Message ID 1595481999-121310-7-git-send-email-hzhou@ovn.org
State Accepted
Headers show
Series Avoid ARP flow explosion. | expand

Commit Message

Han Zhou July 23, 2020, 5:26 a.m. UTC 
In LR ingress stage LOOKUP_NEIGHBOR and LEARN_NEIGHBOR, the flag
REGBIT_SKIP_LOOKUP_NEIGHBOR was used to indicate if mac-binding
lookup can be skipped. This patch avoid using the bit by combining
it with the REGBIT_LOOKUP_NEIGHBOR_RESULT bit, and assigning 1
to REGBIT_LOOKUP_NEIGHBOR_RESULT serves same purpose of skipping
the lookup. There will be a new bit needed in a future patch, and
this change can avoid using too many bits unnecessarily.

Signed-off-by: Han Zhou <hzhou@ovn.org>
---
 northd/ovn-northd.8.xml | 11 ++++-------
 northd/ovn-northd.c     |  6 ++----
 2 files changed, 6 insertions(+), 11 deletions(-)

Comments

Numan Siddique July 29, 2020, 5:37 p.m. UTC | #1 
On Thu, Jul 23, 2020 at 10:57 AM Han Zhou <hzhou@ovn.org> wrote:

> In LR ingress stage LOOKUP_NEIGHBOR and LEARN_NEIGHBOR, the flag
> REGBIT_SKIP_LOOKUP_NEIGHBOR was used to indicate if mac-binding
> lookup can be skipped. This patch avoid using the bit by combining
> it with the REGBIT_LOOKUP_NEIGHBOR_RESULT bit, and assigning 1
> to REGBIT_LOOKUP_NEIGHBOR_RESULT serves same purpose of skipping
> the lookup. There will be a new bit needed in a future patch, and
> this change can avoid using too many bits unnecessarily.
>
> Signed-off-by: Han Zhou <hzhou@ovn.org>
>

Acked-by: Numan Siddique <numans@ovn.org>

Thanks
Numan


> ---
>  northd/ovn-northd.8.xml | 11 ++++-------
>  northd/ovn-northd.c     |  6 ++----
>  2 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 741c928..9f2c70f 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -1595,7 +1595,7 @@ next;
>
>        <li>
>          A priority-0 fallback flow that matches all packets and applies
> -        the action <code>reg9[3] = 1; next;</code>
> +        the action <code>reg9[2] = 1; next;</code>
>          advancing the packet to the next table.
>        </li>
>      </ul>
> @@ -1610,17 +1610,14 @@ next;
>
>      <p>
>        reg9[2] will be <code>1</code> if the
> <code>lookup_arp/lookup_nd</code>
> -      in the previous table was successful.
> -    </p>
> -
> -    <p>
> -      reg9[3] will be <code>1</code> if there was no need to do the
> lookup.
> +      in the previous table was successful, or if there was no need to do
> the
> +      lookup.
>      </p>
>
>      <ul>
>        <li>
>          A priority-100 flow with the match
> -        <code>reg9[2] == 1 || reg9[3] == 1</code> and advances the packet
> +        <code>reg9[2] == 1</code> and advances the packet
>          to the next table as there is no need to learn the neighbor.
>        </li>
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 4e11a25..425f522 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -221,7 +221,6 @@ enum ovn_stage {
>  /* Register to store the result of check_pkt_larger action. */
>  #define REGBIT_PKT_LARGER        "reg9[1]"
>  #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]"
> -#define REGBIT_SKIP_LOOKUP_NEIGHBOR "reg9[3]"
>
>  /* Register to store the eth address associated to a router port for
> packets
>   * received in S_ROUTER_IN_ADMISSION.
> @@ -8275,14 +8274,13 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>                        "lookup_nd(inport, ip6.src, nd.sll); next;");
>
>          /* For other packet types, we can skip neighbor learning.
> -         * So set REGBIT_SKIP_LOOKUP_NEIGHBOR to 1. */
> +         * So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
>          ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 0, "1",
> -                      REGBIT_SKIP_LOOKUP_NEIGHBOR" = 1; next;");
> +                      REGBIT_LOOKUP_NEIGHBOR_RESULT" = 1; next;");
>
>          /* Flows for LEARN_NEIGHBOR. */
>          /* Skip Neighbor learning if not required. */
>          ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 100,
> -                      REGBIT_SKIP_LOOKUP_NEIGHBOR" == 1 || "
>                        REGBIT_LOOKUP_NEIGHBOR_RESULT" == 1", "next;");
>
>          ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,
> --
> 2.1.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Han Zhou July 30, 2020, 6:48 a.m. UTC | #2 
On Wed, Jul 29, 2020 at 10:37 AM Numan Siddique <numans@ovn.org> wrote:
>
>
>
> On Thu, Jul 23, 2020 at 10:57 AM Han Zhou <hzhou@ovn.org> wrote:
>>
>> In LR ingress stage LOOKUP_NEIGHBOR and LEARN_NEIGHBOR, the flag
>> REGBIT_SKIP_LOOKUP_NEIGHBOR was used to indicate if mac-binding
>> lookup can be skipped. This patch avoid using the bit by combining
>> it with the REGBIT_LOOKUP_NEIGHBOR_RESULT bit, and assigning 1
>> to REGBIT_LOOKUP_NEIGHBOR_RESULT serves same purpose of skipping
>> the lookup. There will be a new bit needed in a future patch, and
>> this change can avoid using too many bits unnecessarily.
>>
>> Signed-off-by: Han Zhou <hzhou@ovn.org>
>
>
> Acked-by: Numan Siddique <numans@ovn.org>
>
> Thanks
> Numan
>

Thanks Numan for the review. I applied the patches 2, 3, 5, 6 of this
series, which you have already acked, to master. It will simplify the work
for v2. I will update the rest patches and send v2 after I get all your
feedback.

Thanks,
Han

>>
>> ---
>>  northd/ovn-northd.8.xml | 11 ++++-------
>>  northd/ovn-northd.c     |  6 ++----
>>  2 files changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
>> index 741c928..9f2c70f 100644
>> --- a/northd/ovn-northd.8.xml
>> +++ b/northd/ovn-northd.8.xml
>> @@ -1595,7 +1595,7 @@ next;
>>
>>        <li>
>>          A priority-0 fallback flow that matches all packets and applies
>> -        the action <code>reg9[3] = 1; next;</code>
>> +        the action <code>reg9[2] = 1; next;</code>
>>          advancing the packet to the next table.
>>        </li>
>>      </ul>
>> @@ -1610,17 +1610,14 @@ next;
>>
>>      <p>
>>        reg9[2] will be <code>1</code> if the
<code>lookup_arp/lookup_nd</code>
>> -      in the previous table was successful.
>> -    </p>
>> -
>> -    <p>
>> -      reg9[3] will be <code>1</code> if there was no need to do the
lookup.
>> +      in the previous table was successful, or if there was no need to
do the
>> +      lookup.
>>      </p>
>>
>>      <ul>
>>        <li>
>>          A priority-100 flow with the match
>> -        <code>reg9[2] == 1 || reg9[3] == 1</code> and advances the
packet
>> +        <code>reg9[2] == 1</code> and advances the packet
>>          to the next table as there is no need to learn the neighbor.
>>        </li>
>>
>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>> index 4e11a25..425f522 100644
>> --- a/northd/ovn-northd.c
>> +++ b/northd/ovn-northd.c
>> @@ -221,7 +221,6 @@ enum ovn_stage {
>>  /* Register to store the result of check_pkt_larger action. */
>>  #define REGBIT_PKT_LARGER        "reg9[1]"
>>  #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]"
>> -#define REGBIT_SKIP_LOOKUP_NEIGHBOR "reg9[3]"
>>
>>  /* Register to store the eth address associated to a router port for
packets
>>   * received in S_ROUTER_IN_ADMISSION.
>> @@ -8275,14 +8274,13 @@ build_lrouter_flows(struct hmap *datapaths,
struct hmap *ports,
>>                        "lookup_nd(inport, ip6.src, nd.sll); next;");
>>
>>          /* For other packet types, we can skip neighbor learning.
>> -         * So set REGBIT_SKIP_LOOKUP_NEIGHBOR to 1. */
>> +         * So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
>>          ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 0, "1",
>> -                      REGBIT_SKIP_LOOKUP_NEIGHBOR" = 1; next;");
>> +                      REGBIT_LOOKUP_NEIGHBOR_RESULT" = 1; next;");
>>
>>          /* Flows for LEARN_NEIGHBOR. */
>>          /* Skip Neighbor learning if not required. */
>>          ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 100,
>> -                      REGBIT_SKIP_LOOKUP_NEIGHBOR" == 1 || "
>>                        REGBIT_LOOKUP_NEIGHBOR_RESULT" == 1", "next;");
>>
>>          ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,
>> --
>> 2.1.0
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
Numan Siddique Aug. 3, 2020, 9:33 a.m. UTC | #3 
On Thu, Jul 30, 2020 at 12:18 PM Han Zhou <hzhou@ovn.org> wrote:

> On Wed, Jul 29, 2020 at 10:37 AM Numan Siddique <numans@ovn.org> wrote:
> >
> >
> >
> > On Thu, Jul 23, 2020 at 10:57 AM Han Zhou <hzhou@ovn.org> wrote:
> >>
> >> In LR ingress stage LOOKUP_NEIGHBOR and LEARN_NEIGHBOR, the flag
> >> REGBIT_SKIP_LOOKUP_NEIGHBOR was used to indicate if mac-binding
> >> lookup can be skipped. This patch avoid using the bit by combining
> >> it with the REGBIT_LOOKUP_NEIGHBOR_RESULT bit, and assigning 1
> >> to REGBIT_LOOKUP_NEIGHBOR_RESULT serves same purpose of skipping
> >> the lookup. There will be a new bit needed in a future patch, and
> >> this change can avoid using too many bits unnecessarily.
> >>
> >> Signed-off-by: Han Zhou <hzhou@ovn.org>
> >
> >
> > Acked-by: Numan Siddique <numans@ovn.org>
> >
> > Thanks
> > Numan
> >
>
> Thanks Numan for the review. I applied the patches 2, 3, 5, 6 of this
> series, which you have already acked, to master. It will simplify the work
> for v2. I will update the rest patches and send v2 after I get all your
> feedback.
>

Hi Han,

The other patches look good to me. I don't have any  comments other than
what I provided earlier - Having test cases in ovn-northd.at will be
helpful so that there
are no regressions later.

Thanks
Numan


>
> Thanks,
> Han
>
> >>
> >> ---
> >>  northd/ovn-northd.8.xml | 11 ++++-------
> >>  northd/ovn-northd.c     |  6 ++----
> >>  2 files changed, 6 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> >> index 741c928..9f2c70f 100644
> >> --- a/northd/ovn-northd.8.xml
> >> +++ b/northd/ovn-northd.8.xml
> >> @@ -1595,7 +1595,7 @@ next;
> >>
> >>        <li>
> >>          A priority-0 fallback flow that matches all packets and applies
> >> -        the action <code>reg9[3] = 1; next;</code>
> >> +        the action <code>reg9[2] = 1; next;</code>
> >>          advancing the packet to the next table.
> >>        </li>
> >>      </ul>
> >> @@ -1610,17 +1610,14 @@ next;
> >>
> >>      <p>
> >>        reg9[2] will be <code>1</code> if the
> <code>lookup_arp/lookup_nd</code>
> >> -      in the previous table was successful.
> >> -    </p>
> >> -
> >> -    <p>
> >> -      reg9[3] will be <code>1</code> if there was no need to do the
> lookup.
> >> +      in the previous table was successful, or if there was no need to
> do the
> >> +      lookup.
> >>      </p>
> >>
> >>      <ul>
> >>        <li>
> >>          A priority-100 flow with the match
> >> -        <code>reg9[2] == 1 || reg9[3] == 1</code> and advances the
> packet
> >> +        <code>reg9[2] == 1</code> and advances the packet
> >>          to the next table as there is no need to learn the neighbor.
> >>        </li>
> >>
> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> >> index 4e11a25..425f522 100644
> >> --- a/northd/ovn-northd.c
> >> +++ b/northd/ovn-northd.c
> >> @@ -221,7 +221,6 @@ enum ovn_stage {
> >>  /* Register to store the result of check_pkt_larger action. */
> >>  #define REGBIT_PKT_LARGER        "reg9[1]"
> >>  #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]"
> >> -#define REGBIT_SKIP_LOOKUP_NEIGHBOR "reg9[3]"
> >>
> >>  /* Register to store the eth address associated to a router port for
> packets
> >>   * received in S_ROUTER_IN_ADMISSION.
> >> @@ -8275,14 +8274,13 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
> >>                        "lookup_nd(inport, ip6.src, nd.sll); next;");
> >>
> >>          /* For other packet types, we can skip neighbor learning.
> >> -         * So set REGBIT_SKIP_LOOKUP_NEIGHBOR to 1. */
> >> +         * So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
> >>          ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 0, "1",
> >> -                      REGBIT_SKIP_LOOKUP_NEIGHBOR" = 1; next;");
> >> +                      REGBIT_LOOKUP_NEIGHBOR_RESULT" = 1; next;");
> >>
> >>          /* Flows for LEARN_NEIGHBOR. */
> >>          /* Skip Neighbor learning if not required. */
> >>          ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 100,
> >> -                      REGBIT_SKIP_LOOKUP_NEIGHBOR" == 1 || "
> >>                        REGBIT_LOOKUP_NEIGHBOR_RESULT" == 1", "next;");
> >>
> >>          ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,
> >> --
> >> 2.1.0
> >>
> >> _______________________________________________
> >> 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 mbox series

Patch

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 741c928..9f2c70f 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -1595,7 +1595,7 @@  next;
 
       <li>
         A priority-0 fallback flow that matches all packets and applies
-        the action <code>reg9[3] = 1; next;</code>
+        the action <code>reg9[2] = 1; next;</code>
         advancing the packet to the next table.
       </li>
     </ul>
@@ -1610,17 +1610,14 @@  next;
 
     <p>
       reg9[2] will be <code>1</code> if the <code>lookup_arp/lookup_nd</code>
-      in the previous table was successful.
-    </p>
-
-    <p>
-      reg9[3] will be <code>1</code> if there was no need to do the lookup.
+      in the previous table was successful, or if there was no need to do the
+      lookup.
     </p>
 
     <ul>
       <li>
         A priority-100 flow with the match
-        <code>reg9[2] == 1 || reg9[3] == 1</code> and advances the packet
+        <code>reg9[2] == 1</code> and advances the packet
         to the next table as there is no need to learn the neighbor.
       </li>
 
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 4e11a25..425f522 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -221,7 +221,6 @@  enum ovn_stage {
 /* Register to store the result of check_pkt_larger action. */
 #define REGBIT_PKT_LARGER        "reg9[1]"
 #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]"
-#define REGBIT_SKIP_LOOKUP_NEIGHBOR "reg9[3]"
 
 /* Register to store the eth address associated to a router port for packets
  * received in S_ROUTER_IN_ADMISSION.
@@ -8275,14 +8274,13 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                       "lookup_nd(inport, ip6.src, nd.sll); next;");
 
         /* For other packet types, we can skip neighbor learning.
-         * So set REGBIT_SKIP_LOOKUP_NEIGHBOR to 1. */
+         * So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
         ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 0, "1",
-                      REGBIT_SKIP_LOOKUP_NEIGHBOR" = 1; next;");
+                      REGBIT_LOOKUP_NEIGHBOR_RESULT" = 1; next;");
 
         /* Flows for LEARN_NEIGHBOR. */
         /* Skip Neighbor learning if not required. */
         ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 100,
-                      REGBIT_SKIP_LOOKUP_NEIGHBOR" == 1 || "
                       REGBIT_LOOKUP_NEIGHBOR_RESULT" == 1", "next;");
 
         ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,