| Message ID | 1448826523-6966-1-git-send-email-blp@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On 11/29/2015 02:48 PM, Ben Pfaff wrote: > Until now, the flow table treated localnet logical ports that have a VLAN > quite differently from those that don't. The ones without a VLAN were > essentially trunk ports: any packets that came in, that weren't picked off > by a localnet port with a VLAN, were passed to the ones without a VLAN. > This wasn't the intended behavior. > > This commit changes behavior to the intended behavior. Now, localnet ports > without a specific VLAN only receive packets without a VLAN header or those > with VLAN ID 0 (with that header stripped off). > > Found by inspection. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > Repost of a patch from Oct. 15: > https://patchwork.ozlabs.org/patch/530988/ > > ovn/controller/physical.c | 30 ++++++++++++++++++++---------- > 1 file changed, 20 insertions(+), 10 deletions(-) First of all, sorry for missing this patch earlier. I tried testing this patch and I'm having problems with it. After I apply this patch, if I exercise this code path using part of tutorial/OVN-Tutorial.md, ovn-controller gets stuck and eats a CPU. I actually accidentally got two instances of ovn-controller running in this state and almost melted my laptop. :-) I'm not sure where the error is, but figured I'd at least report my test result. To replicate, just start the sandbox and run env5's setup. $ make sandbox SANDBOXFLAGS="--ovn" $ ovn/env5/setup.sh If I revert this patch, that env works fine again. Yes, I should turn this into a test case. I'm happy to do that once we sort this out. I do like the idea of this patch. I looked at this issue when you mentioned it on IRC one day it seemed less urgent since we do at least drop any VLAN tagged packets at the beginning of the logical flow table. So, it looked like if any unexpectedly tagged packets hit an untagged localnet port's input flow, it would just get dropped as soon as it got to the logical flows. Handling it more explicitly in physical-to-logical translation does seem like a better thing to do, though.
On Mon, Nov 30, 2015 at 09:55:21PM -0500, Russell Bryant wrote: > On 11/29/2015 02:48 PM, Ben Pfaff wrote: > > Until now, the flow table treated localnet logical ports that have a VLAN > > quite differently from those that don't. The ones without a VLAN were > > essentially trunk ports: any packets that came in, that weren't picked off > > by a localnet port with a VLAN, were passed to the ones without a VLAN. > > This wasn't the intended behavior. > > > > This commit changes behavior to the intended behavior. Now, localnet ports > > without a specific VLAN only receive packets without a VLAN header or those > > with VLAN ID 0 (with that header stripped off). > > > > Found by inspection. > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > Repost of a patch from Oct. 15: > > https://patchwork.ozlabs.org/patch/530988/ > > > > ovn/controller/physical.c | 30 ++++++++++++++++++++---------- > > 1 file changed, 20 insertions(+), 10 deletions(-) > > First of all, sorry for missing this patch earlier. > > I tried testing this patch and I'm having problems with it. After I > apply this patch, if I exercise this code path using part of > tutorial/OVN-Tutorial.md, ovn-controller gets stuck and eats a CPU. I > actually accidentally got two instances of ovn-controller running in > this state and almost melted my laptop. :-) > > I'm not sure where the error is, but figured I'd at least report my test > result. > > To replicate, just start the sandbox and run env5's setup. > > $ make sandbox SANDBOXFLAGS="--ovn" > $ ovn/env5/setup.sh > > If I revert this patch, that env works fine again. Thanks for reporting the problem. The problem was a missing call to ofpact_pad() after putting the "strip vlan" here: ofpact_put_STRIP_VLAN(&ofpacts); uint32_t ofpacts_orig_size = ofpacts.size; However, the need for that call is utterly unintuitive. Instead of adding it, I think it's better to get rid of the need for it. So, I posted v2: http://openvswitch.org/pipermail/dev/2015-December/063769.html > Yes, I should turn this into a test case. I'm happy to do that once we > sort this out. We don't currently have any tests for localnet, so that would be greatly appreciated.
On 12/22/2015 04:18 PM, Ben Pfaff wrote: > On Mon, Nov 30, 2015 at 09:55:21PM -0500, Russell Bryant wrote: >> On 11/29/2015 02:48 PM, Ben Pfaff wrote: >>> Until now, the flow table treated localnet logical ports that have a VLAN >>> quite differently from those that don't. The ones without a VLAN were >>> essentially trunk ports: any packets that came in, that weren't picked off >>> by a localnet port with a VLAN, were passed to the ones without a VLAN. >>> This wasn't the intended behavior. >>> >>> This commit changes behavior to the intended behavior. Now, localnet ports >>> without a specific VLAN only receive packets without a VLAN header or those >>> with VLAN ID 0 (with that header stripped off). >>> >>> Found by inspection. >>> >>> Signed-off-by: Ben Pfaff <blp@ovn.org> >>> --- >>> Repost of a patch from Oct. 15: >>> https://patchwork.ozlabs.org/patch/530988/ >>> >>> ovn/controller/physical.c | 30 ++++++++++++++++++++---------- >>> 1 file changed, 20 insertions(+), 10 deletions(-) >> >> First of all, sorry for missing this patch earlier. >> >> I tried testing this patch and I'm having problems with it. After I >> apply this patch, if I exercise this code path using part of >> tutorial/OVN-Tutorial.md, ovn-controller gets stuck and eats a CPU. I >> actually accidentally got two instances of ovn-controller running in >> this state and almost melted my laptop. :-) >> >> I'm not sure where the error is, but figured I'd at least report my test >> result. >> >> To replicate, just start the sandbox and run env5's setup. >> >> $ make sandbox SANDBOXFLAGS="--ovn" >> $ ovn/env5/setup.sh >> >> If I revert this patch, that env works fine again. > > Thanks for reporting the problem. > > The problem was a missing call to ofpact_pad() after putting the "strip > vlan" here: > > ofpact_put_STRIP_VLAN(&ofpacts); > uint32_t ofpacts_orig_size = ofpacts.size; > > However, the need for that call is utterly unintuitive. Instead of > adding it, I think it's better to get rid of the need for it. So, I > posted v2: > http://openvswitch.org/pipermail/dev/2015-December/063769.html > >> Yes, I should turn this into a test case. I'm happy to do that once we >> sort this out. > > We don't currently have any tests for localnet, so that would be greatly > appreciated. > I'm working on a test case now. I'm hoping I can use it to help test out your v2. Thanks,
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c index 5821c11..302bb57 100644 --- a/ovn/controller/physical.c +++ b/ovn/controller/physical.c @@ -726,15 +726,12 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, } hmap_destroy(&tunnels); - /* Table 0, Priority 150 and 100. - * ============================== + /* Table 0, Priority 100. + * ====================== * * We have now determined the full set of port bindings associated with * each "localnet" network. Only create flows for datapaths that have * another local binding. Otherwise, we know it would just be dropped. - * - * Use priority 150 for inputs that match both the network and a VLAN tag. - * Use priority 100 for matching untagged traffic from the local network. */ struct shash_node *ln_bindings_node, *ln_bindings_node_next; SHASH_FOR_EACH_SAFE (ln_bindings_node, ln_bindings_node_next, @@ -747,14 +744,19 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, match_set_in_port(&match, ln_bindings->ofport); if (ln_vlan->tag) { match_set_dl_vlan(&match, htons(ln_vlan->tag)); + } else { + /* 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)); } struct ofpbuf ofpacts; ofpbuf_init(&ofpacts, 0); - if (ln_vlan->tag) { - ofpact_put_STRIP_VLAN(&ofpacts); - } + ofpact_put_STRIP_VLAN(&ofpacts); uint32_t ofpacts_orig_size = ofpacts.size; struct binding_elem *b; @@ -775,8 +777,16 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, } if (ofpacts.size > ofpacts_orig_size) { - ofctrl_add_flow(flow_table, 0, ln_vlan->tag ? 150 : 100, - &match, &ofpacts); + ofctrl_add_flow(flow_table, 0, 100, &match, &ofpacts); + + if (!ln_vlan->tag) { + /* Add a second flow for frames that lack any 802.1Q + * header. For these, drop the OFPACT_STRIP_VLAN + * action. */ + ofpbuf_pull(&ofpacts, ofpacts_orig_size); + match_set_dl_tci_masked(&match, 0, htons(VLAN_CFI)); + ofctrl_add_flow(flow_table, 0, 100, &match, &ofpacts); + } } ofpbuf_uninit(&ofpacts);
Until now, the flow table treated localnet logical ports that have a VLAN quite differently from those that don't. The ones without a VLAN were essentially trunk ports: any packets that came in, that weren't picked off by a localnet port with a VLAN, were passed to the ones without a VLAN. This wasn't the intended behavior. This commit changes behavior to the intended behavior. Now, localnet ports without a specific VLAN only receive packets without a VLAN header or those with VLAN ID 0 (with that header stripped off). Found by inspection. Signed-off-by: Ben Pfaff <blp@ovn.org> --- Repost of a patch from Oct. 15: https://patchwork.ozlabs.org/patch/530988/ ovn/controller/physical.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)