[ovs-dev] [PATCH 3/8] lib/cmap: Simplify iteration with C99 loop declaration.

Jarno Rajahalme jrajahalme at nicira.com
Wed Jun 11 18:15:49 UTC 2014


Thanks for the reviews!

On Jun 10, 2014, at 1:47 PM, Ethan Jackson <ethan at nicira.com> wrote:

> Nice, thing we could do something similar for the classifier loop?
> 

I have it in the pipeline, also integrating locking to the iterators, so that the change to RCU will not be so visible.

> Acked-by: Ethan Jackson <ethan at nicira.com>
> 

Pushed to master (but I forgot to add the Acked-by to the commit message, sorry),

  Jarno


> On Mon, Jun 9, 2014 at 11:53 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>> This further eases porting existing hmap code to use cmap instead.
>> 
>> The iterator variants taking an explicit cursor are retained (renamed)
>> as they are needed when iteration is to be continued from the last
>> iterated node.
>> 
>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
>> ---
>> lib/cmap.h        |   43 ++++++++++++++++++++++++++++++++++++++-----
>> lib/dpif-netdev.c |   15 +++++----------
>> tests/test-cmap.c |   12 +++++-------
>> 3 files changed, 48 insertions(+), 22 deletions(-)
>> 
>> diff --git a/lib/cmap.h b/lib/cmap.h
>> index 2292a75..b7569f5 100644
>> --- a/lib/cmap.h
>> +++ b/lib/cmap.h
>> @@ -167,23 +167,24 @@ struct cmap_node *cmap_find_protected(const struct cmap *, uint32_t hash);
>>  * executing at postponed time, when it is known that the RCU grace period
>>  * has already expired.
>>  */
>> -#define CMAP_FOR_EACH(NODE, MEMBER, CURSOR, CMAP)                       \
>> +
>> +#define CMAP_CURSOR_FOR_EACH(NODE, MEMBER, CURSOR, CMAP)                \
>>     for ((cmap_cursor_init(CURSOR, CMAP),                               \
>>           ASSIGN_CONTAINER(NODE, cmap_cursor_next(CURSOR, NULL), MEMBER)); \
>>          NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);                 \
>>          ASSIGN_CONTAINER(NODE, cmap_cursor_next(CURSOR, &(NODE)->MEMBER), \
>>                           MEMBER))
>> 
>> -#define CMAP_FOR_EACH_SAFE(NODE, NEXT, MEMBER, CURSOR, CMAP)            \
>> +#define CMAP_CURSOR_FOR_EACH_SAFE(NODE, NEXT, MEMBER, CURSOR, CMAP)     \
>>     for ((cmap_cursor_init(CURSOR, CMAP),                               \
>>           ASSIGN_CONTAINER(NODE, cmap_cursor_next(CURSOR, NULL), MEMBER)); \
>>          (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)                  \
>>           ? ASSIGN_CONTAINER(NEXT, cmap_cursor_next(CURSOR, &(NODE)->MEMBER), \
>> -                             MEMBER), 1                                  \
>> -          : 0);                                                          \
>> +                             MEMBER), true                              \
>> +          : false);                                                     \
>>          (NODE) = (NEXT))
>> 
>> -#define CMAP_FOR_EACH_CONTINUE(NODE, MEMBER, CURSOR, CMAP)              \
>> +#define CMAP_CURSOR_FOR_EACH_CONTINUE(NODE, MEMBER, CURSOR)             \
>>     for (ASSIGN_CONTAINER(NODE, cmap_cursor_next(CURSOR, &(NODE)->MEMBER), \
>>                           MEMBER);                                      \
>>          NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);                 \
>> @@ -200,6 +201,38 @@ void cmap_cursor_init(struct cmap_cursor *, const struct cmap *);
>> struct cmap_node *cmap_cursor_next(struct cmap_cursor *,
>>                                    const struct cmap_node *);
>> 
>> +
>> +static inline struct cmap_cursor cmap_cursor_start(const struct cmap *cmap,
>> +                                                   void **pnode,
>> +                                                   const void *offset)
>> +{
>> +    struct cmap_cursor cursor;
>> +
>> +    cmap_cursor_init(&cursor, cmap);
>> +    *pnode = (char *)cmap_cursor_next(&cursor, NULL) + (ptrdiff_t)offset;
>> +
>> +    return cursor;
>> +}
>> +
>> +#define CMAP_CURSOR_START(NODE, MEMBER, CMAP)                   \
>> +    cmap_cursor_start(CMAP, (void **)&(NODE),                   \
>> +                      OBJECT_CONTAINING(NULL, NODE, MEMBER))
>> +
>> +#define CMAP_FOR_EACH(NODE, MEMBER, CMAP)                               \
>> +    for (struct cmap_cursor cursor__ = CMAP_CURSOR_START(NODE, MEMBER, CMAP); \
>> +         NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);                 \
>> +         ASSIGN_CONTAINER(NODE, cmap_cursor_next(&cursor__, &(NODE)->MEMBER), \
>> +                          MEMBER))
>> +
>> +#define CMAP_FOR_EACH_SAFE(NODE, NEXT, MEMBER, CMAP)                    \
>> +    for (struct cmap_cursor cursor__ = CMAP_CURSOR_START(NODE, MEMBER, CMAP); \
>> +         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)                 \
>> +          ? ASSIGN_CONTAINER(NEXT,                                      \
>> +                             cmap_cursor_next(&cursor__, &(NODE)->MEMBER), \
>> +                             MEMBER), true                              \
>> +          : false);                                                     \
>> +         (NODE) = (NEXT))
>> +
>> static inline struct cmap_node *cmap_first(const struct cmap *);
>> 
>> /* Another, less preferred, form of iteration, for use in situations where it
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 27fe4fc..d4627b2 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -537,7 +537,6 @@ dp_netdev_free(struct dp_netdev *dp)
>> {
>>     struct dp_netdev_port *port;
>>     struct dp_netdev_stats *bucket;
>> -    struct cmap_cursor cursor;
>>     int i;
>> 
>>     shash_find_and_delete(&dp_netdevs, dp->name);
>> @@ -547,7 +546,7 @@ dp_netdev_free(struct dp_netdev *dp)
>> 
>>     dp_netdev_flow_flush(dp);
>>     ovs_mutex_lock(&dp->port_mutex);
>> -    CMAP_FOR_EACH (port, node, &cursor, &dp->ports) {
>> +    CMAP_FOR_EACH (port, node, &dp->ports) {
>>         do_del_port(dp, port);
>>     }
>>     ovs_mutex_unlock(&dp->port_mutex);
>> @@ -848,9 +847,8 @@ get_port_by_name(struct dp_netdev *dp,
>>     OVS_REQUIRES(dp->port_mutex)
>> {
>>     struct dp_netdev_port *port;
>> -    struct cmap_cursor cursor;
>> 
>> -    CMAP_FOR_EACH (port, node, &cursor, &dp->ports) {
>> +    CMAP_FOR_EACH (port, node, &dp->ports) {
>>         if (!strcmp(netdev_get_name(port->netdev), devname)) {
>>             *portp = port;
>>             return 0;
>> @@ -1772,9 +1770,8 @@ dpif_netdev_run(struct dpif *dpif)
>> {
>>     struct dp_netdev_port *port;
>>     struct dp_netdev *dp = get_dp_netdev(dpif);
>> -    struct cmap_cursor cursor;
>> 
>> -    CMAP_FOR_EACH (port, node, &cursor, &dp->ports) {
>> +    CMAP_FOR_EACH (port, node, &dp->ports) {
>>         if (!netdev_is_pmd(port->netdev)) {
>>             int i;
>> 
>> @@ -1790,10 +1787,9 @@ dpif_netdev_wait(struct dpif *dpif)
>> {
>>     struct dp_netdev_port *port;
>>     struct dp_netdev *dp = get_dp_netdev(dpif);
>> -    struct cmap_cursor cursor;
>> 
>>     ovs_mutex_lock(&dp_netdev_mutex);
>> -    CMAP_FOR_EACH (port, node, &cursor, &dp->ports) {
>> +    CMAP_FOR_EACH (port, node, &dp->ports) {
>>         if (!netdev_is_pmd(port->netdev)) {
>>             int i;
>> 
>> @@ -1817,7 +1813,6 @@ pmd_load_queues(struct pmd_thread *f,
>>     struct dp_netdev *dp = f->dp;
>>     struct rxq_poll *poll_list = *ppoll_list;
>>     struct dp_netdev_port *port;
>> -    struct cmap_cursor cursor;
>>     int id = f->id;
>>     int index;
>>     int i;
>> @@ -1830,7 +1825,7 @@ pmd_load_queues(struct pmd_thread *f,
>>     poll_cnt = 0;
>>     index = 0;
>> 
>> -    CMAP_FOR_EACH (port, node, &cursor, &f->dp->ports) {
>> +    CMAP_FOR_EACH (port, node, &f->dp->ports) {
>>         if (netdev_is_pmd(port->netdev)) {
>>             int i;
>> 
>> diff --git a/tests/test-cmap.c b/tests/test-cmap.c
>> index f8f68b4..46c00e1 100644
>> --- a/tests/test-cmap.c
>> +++ b/tests/test-cmap.c
>> @@ -56,8 +56,7 @@ check_cmap(struct cmap *cmap, const int values[], size_t n,
>>            hash_func *hash)
>> {
>>     int *sort_values, *cmap_values;
>> -    struct cmap_cursor cursor;
>> -    struct element *e;
>> +    const struct element *e;
>>     size_t i;
>> 
>>     /* Check that all the values are there in iteration. */
>> @@ -65,7 +64,7 @@ check_cmap(struct cmap *cmap, const int values[], size_t n,
>>     cmap_values = xmalloc(sizeof *sort_values * n);
>> 
>>     i = 0;
>> -    CMAP_FOR_EACH (e, node, &cursor, cmap) {
>> +    CMAP_FOR_EACH (e, node, cmap) {
>>         assert(i < n);
>>         cmap_values[i++] = e->value;
>>     }
>> @@ -119,7 +118,7 @@ print_cmap(const char *name, struct cmap *cmap)
>>     struct element *e;
>> 
>>     printf("%s:", name);
>> -    CMAP_FOR_EACH (e, node, &cursor, cmap) {
>> +    CMAP_CURSOR_FOR_EACH (e, node, &cursor, cmap) {
>>         printf(" %d", e->value);
>>     }
>>     printf("\n");
>> @@ -296,7 +295,6 @@ benchmark_cmap(void)
>>     struct element *elements;
>>     struct cmap cmap;
>>     struct element *e;
>> -    struct cmap_cursor cursor;
>>     struct timeval start;
>>     pthread_t *threads;
>>     struct cmap_aux aux;
>> @@ -315,7 +313,7 @@ benchmark_cmap(void)
>> 
>>     /* Iteration. */
>>     xgettimeofday(&start);
>> -    CMAP_FOR_EACH (e, node, &cursor, &cmap) {
>> +    CMAP_FOR_EACH (e, node, &cmap) {
>>         ignore(e);
>>     }
>>     printf("cmap iterate: %5d ms\n", elapsed(&start));
>> @@ -336,7 +334,7 @@ benchmark_cmap(void)
>> 
>>     /* Destruction. */
>>     xgettimeofday(&start);
>> -    CMAP_FOR_EACH (e, node, &cursor, &cmap) {
>> +    CMAP_FOR_EACH (e, node, &cmap) {
>>         cmap_remove(&cmap, &e->node, hash_int(e->value, 0));
>>     }
>>     cmap_destroy(&cmap);
>> --
>> 1.7.10.4
>> 
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list