[ovs-dev] [PATCH 3/3] lacp: Implement custom timing mode.

Ben Pfaff blp at nicira.com
Tue Apr 19 18:22:19 UTC 2011

On Tue, Apr 19, 2011 at 10:49:22AM -0700, Ethan Jackson wrote:
> > It seems to me that there are several instances of code that is
> > something like
> > ? lacp_time == LACP_TIME_CUSTOM ? lacp->custom_time
> > ? : lacp_time == LACP_TIME_FAST ? LACP_FAST_TIME_TX
> > I'd suggest using a helper function or a member in struct lacp to
> > avoid the repetition. ?Even two helpers or members, if you want both rx
> > and tx rates.
> I actually thought about doing this, but it turns out that we only do
> this in the code once.  There are three sections of the code I think
> you are referencing.  The first is a switch statement in
> lacp_process_pdu which does exactly as you describe.  The second is in
> lacp_run().  It's similar to what you describe, however if the timing
> mode isn't LACP_TIME_CUSTOM then it chooses FAST or SLOW time based on
> its *partners* state, not based on the lacp_time field in the lacp
> structure. The third is in slave_set_expired() which uses always uses
> LACP_FAST_TIME unless in custom timing mode.  Each case is different
> enough that I don't think they can be cleanly shoved into one
> function.  I may be wrong though, I can think about it more.

How about helpers something like this:

    static long long int
    tx_duration(const struct lacp *lacp, bool fast)
	return (lacp->lacp_time == LACP_TIME_CUSTOM ? lacp->custom_time
		: fast ? LACP_FAST_TIME_TX

    static long long int
    rx_duration(const struct lacp *lacp, bool fast)
	return rx_duration(lacp, fast) * LACP_RX_MULTIPLIER;

and then in lacp_process_pdu():

    tx_rate = tx_duration(lacp, lacp->lacp_time == LACP_TIME_FAST);

and then in lacp_run():

    duration = tx_duration(lacp, slave->partner.state & LACP_STATE_TIME);

and in slave_set_expired():

    duration = rx_duration(lacp, lacp->lacp_time == LACP_TIME_FAST);
    timer_set_duration(&slave->rx, duration);



More information about the dev mailing list