Sent from my iPhone
> On Jul 19, 2014, at 10:03 AM, Ben Pfaff <b...@nicira.com> wrote:
>
>> On Fri, Jul 18, 2014 at 11:33:38PM -0700, Ben Pfaff wrote:
>>> On Fri, Jul 18, 2014 at 09:05:49PM -0700, Jarno Rajahalme wrote:
>>> Older GCC (e.g. 4.1.2) did not like the pointer casts used for
>>> initializing the iteration cursors. This patch changes the code to
>>> avoid the void casts for GCC, and makes the classifier interface more
>>> similar to that of the cmap. These changes make the code work with
>>> GCC 4.1.2 strict aliasing rules.
>>>
>>> VMware-BZ: #1287651
>>>
>>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>>
>> I see why GCC didn't like this code.
>>
>> There are lots of tricky constraints here. We don't want to violate ISO
>> aliasing rules, we have to stick to exactly one declaration in the first
>> clause of the for statement, and we don't want to have different code
>> for GCC and other compilers if we don't have to. The proposed solution
>> here compromises on the last one.
>>
>> I spent a bunch of time experimenting and came up with what I think is
>> the outline of another solution. How about adding a 'node' member to
>> cmap_cursor, like this:
>>
>> struct cmap_cursor {
>> const struct cmap_impl *impl;
>> uint32_t bucket_idx;
>> int entry_idx;
>> struct cmap_node *node;
>> };
>>
>> struct cmap_cursor cmap_cursor_start(const struct cmap *);
>> void cmap_cursor_advance(struct cmap_cursor *);
>>
>> #define CMAP_FOR_EACH(NODE, MEMBER, CMAP) \
>> for (struct cmap_cursor cursor__ = cmap_cursor_start(CMAP); \
>> (cursor__.node \
>> ? (ASSIGN_CONTAINER(NODE, cursor__.node, MEMBER), true) \
>> : false); \
>> cmap_cursor_advance(&cursor__))
>>
>> #define CMAP_FOR_EACH_SAFE(NODE, MEMBER, CMAP) \
>> for (struct cmap_cursor cursor__ = cmap_cursor_start(CMAP); \
>> (cursor__.node \
>> ? (ASSIGN_CONTAINER(NODE, cursor__.node, MEMBER), \
>> cmap_cursor_advance(&cursor__), \
>> true) \
>> : false); \
>> )
>>
>> with corresponding support functions:
>>
>> struct cmap_cursor
>> cmap_cursor_start(const struct cmap *cmap)
>> {
>> struct cmap_cursor cursor;
>>
>> cursor.impl = cmap_get_impl(cmap);
>> cursor.bucket_idx = 0;
>> cursor.entry_idx = 0;
>> cursor.node = NULL;
>> cmap_cursor_advance(&cursor);
>>
>> return cursor;
>> }
>>
>> void
>> cmap_cursor_advance(struct cmap_cursor *cursor)
>> {
>> const struct cmap_impl *impl = cursor->impl;
>>
>> if (cursor->node) {
>> cursor->node = cmap_node_next(cursor->node);
>> if (cursor->node) {
>> return;
>> }
>> }
>>
>> while (cursor->bucket_idx <= impl->mask) {
>> const struct cmap_bucket *b = &impl->buckets[cursor->bucket_idx];
>>
>> while (cursor->entry_idx < CMAP_K) {
>> cursor->node = cmap_node_next(&b->nodes[cursor->entry_idx++]);
>> if (cursor->node) {
>> return;
>> }
>> }
>>
>> cursor->bucket_idx++;
>> cursor->entry_idx = 0;
>> }
>> }
>
> Here's a fully worked out version of this idea that compiles and passes
> all the tests (at least with GCC 4.7.2). With this, the _SAFE variants
> don't take an extra named variable or really have any extra cost, so one
> thinks about just getting making the normal variant the same as _SAFE:
>
Just a quick note, more later: Corresponding changes to classifier would be
more complex, and there is a locking difference between the normal and safe
cases. Did you consider if classifier should use this same pattern?
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
