[ovs-dev] [bond megaflow v4 2/4] ofproto-dpif: Added Per backer recirculation ID management

[Andy Zhou]

On Tue, Mar 25, 2014 at 8:35 AM, Ben Pfaff <blp at nicira.com> wrote:
> On Mon, Mar 24, 2014 at 06:58:42PM -0700, Andy Zhou wrote:
>> Recirculation ID needs to be unique per datapath. Its usage will be
>> tracked by the backer that corresponds to the datapath.
>>
>> In theory, Recirculation ID can be any uint32_t value, except 0. This
>> implementation limits to a smaller range just for ease of debugging.
>> Make the range size 0 effectively disables recirculation.
>>
>> Signed-off-by: Andy Zhou <azhou at nicira.com>
>
> There's some trailing whitespace:
>     /home/blp/nicira/ovs/.git/rebase-apply/patch:79: trailing whitespace.
>         struct ovs_mutex lock;
>     /home/blp/nicira/ovs/.git/rebase-apply/patch:261: trailing whitespace.
>      * ID pool keeps track recirculation ids.
>     /home/blp/nicira/ovs/.git/rebase-apply/patch:273: trailing whitespace.
>      *
>     warning: 3 lines add whitespace errors.
>
> Missing {} here in rid_pool_alloc_id():
> +    if (rids->n_ids == 0)
> +        return 0;
> also here:
> +        if ((rid_pool_find(rids, id)))
> +            goto found_free_id;
> Extra () here in rid_pool_alloc_id():
> +    if (!(rid_pool_find(rids, rids->next_free_id))) {
> and here:
> +        if ((rid_pool_find(rids, id)))
>
> It looks like open_dpif_backer() creates the recirculation pool after it
> starts udpif threads.  It might be a good idea to use the other order (I
> guess receiving packets could eventually trigger recirc id allocation).
>
> Acked-by: Ben Pfaff <blp at nicira.com>
>
Thanks. I will apply with following fixes:

diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
index e464e4c..9e7e9a3 100644
--- a/ofproto/ofproto-dpif-rid.c
+++ b/ofproto/ofproto-dpif-rid.c
@@ -38,7 +38,7 @@ struct rid_pool {
 };

 struct recirc_id_pool {
-    struct ovs_mutex lock;
+    struct ovs_mutex lock;
     struct rid_pool rids;
 };

@@ -146,8 +146,9 @@ rid_pool_alloc_id(struct rid_pool *rids)
 {
     uint32_t id;

-    if (rids->n_ids == 0)
+    if (rids->n_ids == 0) {
         return 0;
+    }

     if (!(rid_pool_find(rids, rids->next_free_id))) {
         id = rids->next_free_id;
@@ -155,8 +156,9 @@ rid_pool_alloc_id(struct rid_pool *rids)
     }

     for(id = rids->base; id < rids->base + rids->n_ids; id++) {
-        if ((rid_pool_find(rids, id)))
+        if (rid_pool_find(rids, id)) {
             goto found_free_id;
+        }
     }

     /* Not available. */
diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
index 4020205..3344e2a 100644
--- a/ofproto/ofproto-dpif-rid.h
+++ b/ofproto/ofproto-dpif-rid.h
@@ -27,7 +27,7 @@ struct recirc_id_pool;
  * ======================
  *
  * Recirculation ID needs to be unique for each datapath. Recirculation
- * ID pool keeps track recirculation ids.
+ * ID pool keeps track recirculation ids.
  *
  * Typically, there is one recirculation ID pool for each backer.
  *
@@ -39,7 +39,7 @@ struct recirc_id_pool;
  * =============
  *
  * All APIs are thread safe.
- *
+ *
  */
 struct recirc_id_pool *recirc_id_pool_create(void);
 void  recirc_id_pool_destroy(struct recirc_id_pool *pool);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 56e0ed7..87a61f7 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -898,13 +898,12 @@ open_dpif_backer(const char *type, struct
dpif_backer **backerp)
     }
     backer->variable_length_userdata = check_variable_length_userdata(backer);
     backer->max_mpls_depth = check_max_mpls_depth(backer);
+    backer->rid_pool = recirc_id_pool_create();

     if (backer->recv_set_enable) {
         udpif_set_threads(backer->udpif, n_handlers, n_revalidators);
     }

-    backer->rid_pool = recirc_id_pool_create();
-
     return error;
 }