On Sat, Jul 19, 2014 at 11:33:36AM +0300, Jarno Rajahalme wrote:
>
>
> Sent from my iPhone
>
> > On Jul 19, 2014, at 10:03 AM, Ben Pfaff <[email protected]> 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 <[email protected]>
> >>
> >> 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?
It doesn't seem to be hard to use that pattern there. I sent out a
2-patch series:
http://openvswitch.org/pipermail/dev/2014-July/042902.html
http://openvswitch.org/pipermail/dev/2014-July/042901.html
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
