This patch currently causes MPLS unittests failure, because it treats all
pkts with 'recirc_id' as bond traffic. I'm thinking if we could reserve a
range of 'recirc_id' for bonding so that 'recirc_id' used for other traffic
will be unaffected,
Anyway, hope to hear any comments,~
Thanks,
Alex Wang,
On Wed, Dec 17, 2014 at 1:19 PM, Alex Wang <al...@nicira.com> wrote:
>
> After commit 0c7812e5e (recirculation: Do not drop packet when
> there is no match from internal table.), if flow keys are modified
> before the recirculation action (e.g. set vlan ID), the miss
> handling of recirculated packets may take different path than the
> ones.
>
> This commit adds an unittest that captures this bug. Moreover,
> to solve this bug, this commit makes OVS directly input the
> recirculated packets from local port of the bridge that owns
> the correpsonding bond. Thus, those packets are always handled
> from the correct bridge.
>
> Signed-off-by: Alex Wang <al...@nicira.com>
> ---
> ofproto/bond.c | 26 +++++++++++++++++++
> ofproto/bond.h | 2 ++
> ofproto/ofproto-dpif-xlate.c | 20 ++++++++++++--
> tests/ofproto-dpif.at | 59
> ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 105 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/bond.c b/ofproto/bond.c
> index c4b3a3a..08900fa 100644
> --- a/ofproto/bond.c
> +++ b/ofproto/bond.c
> @@ -254,6 +254,32 @@ bond_ref(const struct bond *bond_)
> return bond;
> }
>
> +struct ofproto_dpif *
> +bond_get_ofproto(const struct bond *bond)
> +{
> + return bond->ofproto;
> +}
> +
> +/* Searches bond that uses 'recirc_id'. Returns an 'ref_count'ed
> + * reference to 'bond'. */
> +struct bond *
> +bond_find_by_recirc_id(const uint32_t recirc_id)
> +{
> + struct bond *bond;
> +
> + ovs_rwlock_rdlock(&rwlock);
> + HMAP_FOR_EACH (bond, hmap_node, all_bonds) {
> + if (bond->recirc_id == recirc_id) {
> + bond_ref(bond);
> + ovs_rwlock_unlock(&rwlock);
> + return bond;
> + }
> + }
> + ovs_rwlock_unlock(&rwlock);
> +
> + return NULL;
> +}
> +
> /* Frees 'bond'. */
> void
> bond_unref(struct bond *bond)
> diff --git a/ofproto/bond.h b/ofproto/bond.h
> index c7b6308..43600df 100644
> --- a/ofproto/bond.h
> +++ b/ofproto/bond.h
> @@ -68,6 +68,8 @@ struct bond *bond_create(const struct bond_settings *,
> struct ofproto_dpif *ofproto);
> void bond_unref(struct bond *);
> struct bond *bond_ref(const struct bond *);
> +struct bond *bond_find_by_recirc_id(const uint32_t recirc_id);
> +struct ofproto_dpif *bond_get_ofproto(const struct bond*);
>
> bool bond_reconfigure(struct bond *, const struct bond_settings *);
> void bond_slave_register(struct bond *, void *slave_, ofp_port_t ofport,
> struct netdev *);
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index c1327a6..dd0f329 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -981,9 +981,25 @@ static struct ofproto_dpif *
> xlate_lookup_ofproto_(const struct dpif_backer *backer, const struct flow
> *flow,
> ofp_port_t *ofp_in_port, const struct xport
> **xportp)
> {
> - const struct xport *xport;
> + const struct xport *xport = NULL;
> +
> + /* If 'flow->recirc_id' is set, uses the OFPP_LOCAL port of the
> + * 'xbridge' that owns the bond. Using 'OFPP_LOCAL' should be fine,
> + * since the lookup only matches 'recirc_id' and 'dp_hash'. */
> + if (flow->recirc_id) {
> + struct bond *bond = bond_find_by_recirc_id(flow->recirc_id);
> +
> + if (bond) {
> + struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *,
> &xcfgp);
> + struct ofproto_dpif *ofproto = bond_get_ofproto(bond);
>
> - *xportp = xport = xlate_lookup_xport(backer, flow);
> + bond_unref(bond);
> + *xportp = xport = get_ofp_port(xbridge_lookup(xcfg, ofproto),
> + OFPP_LOCAL);
> + }
> + } else {
> + *xportp = xport = xlate_lookup_xport(backer, flow);
> + }
>
> if (xport) {
> if (ofp_in_port) {
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index baa942f..5e4e245 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -144,6 +144,65 @@ AT_CHECK([ovs-appctl dpif/dump-flows br1 |grep tcp >
> br1_flows.txt])
> AT_CHECK([test `grep in_port.4 br1_flows.txt |wc -l` -gt 24])
> AT_CHECK([test `grep in_port.5 br1_flows.txt |wc -l` -gt 24])
> AT_CHECK([test `grep in_port.6 br1_flows.txt |wc -l` -gt 24])
> +
> +OVS_VSWITCHD_STOP()
> +AT_CLEANUP
> +
> +# Makes sure recirculation does not change the way packet is handled.
> +AT_SETUP([ofproto-dpif, balance-tcp bonding, different recirc flow ])
> +OVS_VSWITCHD_START(
> + [add-bond br0 bond0 p1 p2 bond_mode=balance-tcp lacp=active \
> + other-config:lacp-time=fast other-config:bond-rebalance-interval=0
> -- \
> + set interface p1 type=dummy options:pstream=punix:$OVS_RUNDIR/p1.sock
> ofport_request=1 -- \
> + set interface p2 type=dummy options:pstream=punix:$OVS_RUNDIR/p2.sock
> ofport_request=2 -- \
> + add-br br1 -- \
> + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
> + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \
> + fail-mode=standalone -- \
> + add-bond br1 bond1 p3 p4 bond_mode=balance-tcp lacp=active \
> + other-config:lacp-time=fast other-config:bond-rebalance-interval=0
> -- \
> + set interface p3 type=dummy options:stream=unix:$OVS_RUNDIR/p1.sock
> ofport_request=3 -- \
> + set interface p4 type=dummy options:stream=unix:$OVS_RUNDIR/p2.sock
> ofport_request=4 -- \
> + add-port br1 br1- -- set interface br1- type=patch options:peer=br1+
> ofport_request=100 -- \
> + set port br1- tag=1 -- \
> + add-br br-int -- \
> + set bridge br-int other-config:hwaddr=aa:77:aa:77:00:00 -- \
> + set bridge br-int datapath-type=dummy other-config:datapath-id=1235 \
> + fail-mode=secure -- \
> + add-port br-int br1+ -- set interface br1+ type=patch
> options:peer=br1- ofport_request=101 -- \
> + add-port br-int p5 -- set interface p5 ofport_request=5 type=dummy
> +])
> +AT_CHECK([ovs-appctl netdev-dummy/set-admin-state up], 0, [OK
> +])
> +
> +# The dl_vlan flow should not be ever matched,
> +# since recirculation should not change the flow handling.
> +AT_DATA([flows.txt], [dnl
> +table=0 priority=1 in_port=5 actions=output(101)
> +table=0 priority=2 in_port=5 dl_vlan=1 actions=drop
> +])
> +AT_CHECK([ovs-ofctl add-flows br-int flows.txt])
> +
> +# Sends a packet to trigger recirculation.
> +# Should generate recirc_id(0x12d),dp_hash(0xc1261ba2/0xff).
> +AT_CHECK([ovs-appctl netdev-dummy/receive p5
> "in_port(5),eth(src=50:54:00:00:00:05,dst=50:54:00:00:01:00),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1)"])
> +
> +# Collects flow stats.
> +AT_CHECK([ovs-appctl revalidator/purge], [0])
> +
> +# Checks the flow stats in br1, should only be one flow with non-zero
> +# 'n_packets' from internal table.
> +AT_CHECK([ovs-appctl bridge/dump-flows br1 | ofctl_strip | grep --
> "n_packets" | grep -- "table_id" | sed -e 's/output:[[0-9]]\+/output/'],
> [0], [dnl
> +table_id=254, n_packets=1, n_bytes=64,
> priority=20,recirc_id=0x12d,dp_hash=0xa2/0xff,actions=output
> +])
> +
> +# Checks the flow stats in br-int, should be only one match.
> +AT_CHECK([ovs-ofctl dump-flows br-int | ofctl_strip | sort], [0], [dnl
> + n_packets=1, n_bytes=60, priority=1,in_port=5 actions=output:101
> + priority=2,in_port=5,dl_vlan=1 actions=drop
> +NXST_FLOW reply:
> +])
> +
> OVS_VSWITCHD_STOP()
> AT_CLEANUP
>
> --
> 1.7.9.5
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
