On Wed, Jan 20, 2016 at 10:40:56PM -0800, Han Zhou wrote:
> On Wed, Jan 20, 2016 at 3:22 PM, Ben Pfaff <b...@ovn.org> wrote:
> >
> > On Wed, Jan 20, 2016 at 11:31:08AM -0500, Russell Bryant wrote:
> > > Multiple logical ports on the same chassis that were connected to the
> > > same physical network via localnet ports were not able to send packets
> > > to each other. This was because ovn-controller created a single patch
> > > port between br-int and the physical network access bridge and used it
> > > for all localnet ports.
> > >
> > > The fix implemented here is to create a separate patch port for every
> > > logical port of type=localnet. An optimization is included where these
> > > ports are only created if the localnet port is on a logical switch with
> > > another logical port with an associated local VIF.
> > >
> > > A nice side effect of this fix is that the code in physical.c got a lot
> > > simpler, as localnet ports are now handled mostly like local VIFs.
> > >
> > > Fixes: c02819293d52 ("ovn: Add "localnet" logical port type.")
> > > Reported-by: Han Zhou <zhou...@gmail.com>
> > > Reported-at:
> http://openvswitch.org/pipermail/dev/2016-January/064413.html
> > > Signed-off-by: Russell Bryant <russ...@ovn.org>
> > > Tested-by: Kyle Mestery <mest...@mestery.com
> > > Acked-By: Kyle Mestery <mest...@mestery.com>
> > > ---
> > >
> > > v1->v2:
> > > - updated for feedback from Han Zhou.
> >
> > I guess I reviewed v1 accidentally. Here's a copy of my feedback from
> > that version.
> >
> > The new logic here in physical_run() looks wrong to me. I think that it
> > strips a vlan if it didn't match on one.
> >
> > if (tag) {
> > match_set_dl_vlan(&match, htons(tag));
> > } else if (!strcmp(binding->type, "localnet")) {
> > /* Match priority-tagged frames, e.g. VLAN ID 0.
> > *
> > * We'll add a second flow for frames that lack any 802.1Q
> > * header later. */
> > match_set_dl_tci_masked(&match, htons(VLAN_CFI),
> > htons(VLAN_VID_MASK | VLAN_CFI));
> > }
> >
> > /* Strip vlans. */
> > if (tag) {
> > ofpact_put_STRIP_VLAN(&ofpacts);
> > }
> >
> > I believe that we can simplify it to the following, while fixing the bug
> > at the same time:
> >
> > /* Match a VLAN tag and strip it, including stripping
> priority tags
> > * (e.g. VLAN ID 0). In the latter case we'll add a second
> flow
> > * for frames that lack any 802.1Q header later. */
> > if (tag || !strcmp(binding->type, "localnet")) {
> > match_set_dl_vlan(&match, htons(tag));
> > ofpact_put_STRIP_VLAN(&ofpacts);
> > }
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
> Hi Ben,
>
> Do you mean even if tag == 0, we should still strip 802.1Q header? That
> make sense, and then it is an existed bug even before this patch because
> the new logic of this patch is equivalent to the old logic. It will not add
> STRIP_VLAN action if vlan tag is not matched.
The desired behavior for 'tag == 0' is to add two flows:
* One that matches on a priority-tagged VLAN header and strips it.
* One that matches on no VLAN header at all.
I'm pretty sure that the existing code has this behavior already; it
looks correct to me.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
