Thanks for the reviews!
On Jun 10, 2014, at 1:47 PM, Ethan Jackson <et...@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 <et...@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 <jrajaha...@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 <jrajaha...@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@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
