On Nov 6, 2014, at 7:30 AM, Daniele Venturino <daniele.ventur...@m3s.it> wrote:
> Hi.
> We tested the RSTP implementation against the IXIA IxANVL validation software.
> This allowed us to fix some small bugs, and the implementation now passes
> such tests with success!
> Please find attached the patch series.
>
>
> It appears that patches 12 and 17 break the "RSTP dummy" test, while all other
> tests succeed. We also did some tests on some physical machines and the
> behaviour
> seems to be ok with patches 12 and 17 applied.
I see that after patch 12 RSTP no longer moves to the forwarding state when the
ports are put up.
Based on the old behavior, the test expects to see these transitions after also
the p2 admin-state is changed to ‘up’:
port p2: RSTP state changed from Discarding to Forwarding
port p1: RSTP state changed from Discarding to Forwarding
But this no longer happens.
During the test we have the time stopped, so no timeouts happen. If I add these
after p2 admin-state change, the test will succeed:
# give time for RSTP to move initially
ovs-appctl time/warp 1000
ovs-appctl time/warp 1000
I guess patch 12 adding the the following lines to the p2 admin-state change
event changes the state machine behavior enough to require this.
+ struct rstp * rstp = p->rstp;
+ rstp->changes = true;
+ move_rstp__(rstp);
However, I don’t see why adding this would make the state progression slower or
to require more steps, as previously the RSTP states transitioned to Forwarding
without the added code or the added time warps.
Also, in general, I think it would be nice to give some rationale for each
change in the commit message. In this case, for example, I have no idea why
moving the state machines is necessary in this case, especially when it seems
to have counterintuitive effects. Instead of re-spinning the whole series, how
about you give me one-line justification for each patch that already does not
have one? It would be nice if you could refer to a spec, conformance test
result, etc.
Regards,
Jarno
>
> The "RSTP dummy" test succeeds with the following modifications:
>
> diff --git a/lib/rstp.c b/lib/rstp.c
> index 144f2ba..6803bb2 100644
> --- a/lib/rstp.c
> +++ b/lib/rstp.c
> @@ -324,10 +324,9 @@ rstp_set_bridge_address__(struct rstp *rstp,
> rstp_identifier bridge_address)
> RSTP_ID_ARGS(bridge_address));
> if (rstp->address != bridge_address) {
> rstp->address = bridge_address;
> - rstp->bridge_identifier &= 0xffff000000000000ULL;
> - rstp->bridge_identifier |= bridge_address;
> - set_bridge_priority__(rstp);
> }
> + rstp->bridge_identifier = rstp->address;
> + set_bridge_priority__(rstp);
> }
>
> /* Sets the bridge address. */
> @@ -378,10 +377,10 @@ rstp_set_bridge_priority__(struct rstp *rstp, int
> new_priority)
> VLOG_DBG("%s: set bridge priority to %d", rstp->name, new_priority);
>
> rstp->priority = new_priority;
> - rstp->bridge_identifier &= 0x0000ffffffffffffULL;
> - rstp->bridge_identifier |= (uint64_t)new_priority << 48;
> - set_bridge_priority__(rstp);
> }
> + rstp->bridge_identifier &= 0x0000ffffffffffffULL;
> + rstp->bridge_identifier |= (uint64_t)rstp->priority << 48;
> + set_bridge_priority__(rstp);
> }
>
> but this breaks some IXIA validation tests.
> We're still not sure why this test fails. Jarno, could you take a look?
>
> Best regards,
> Daniele
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
