[ovs-dev] [PATCHv2 2/5] ofproto-dpif: Don't poll ports when nothing changes

Ben Pfaff blp at nicira.com
Wed Nov 20 19:15:06 UTC 2013


On Thu, Nov 14, 2013 at 03:28:26PM -0800, Joe Stringer wrote:
> Currently, as part of ofproto-dpif run() processing, we loop through all
> ports and poll corresponding devices for changes in carrier, cfm and bfd
> status. This allows us to determine how it may affect bundles. For the
> average case where devices are not going up or down constantly, this is
> a large amount of unnecessary processing.
> 
> This patch gets the bfd, cfm, lacp and stp modules to update the global
> netdev change_seq when changes in port/device status occur. We can then
> use this global change_seq to check if anything has changed before
> looping through the ports in ofproto-dpif. In a test environment of 5000
> internal ports and 50 tunnel ports with bfd, this reduces CPU usage from
> about 35% to about 25%.
> 
> Signed-off-by: Joe Stringer <joestringer at nicira.com>

I think that this introduces a strict but easy to miss dependency on the
order in which ofproto_run() calls the provider's 'run' function and
that it updates the ofproto's change_seq: it works with the current
order but if the order were reversed then it would break.  I think it
would be better to avoid this dependency.  (One way would be for
ofproto_dpif to maintain its own change sequence number.)

I think that it is a little surprising to use a netdev sequence number
to track changes to things that are not network devices.  If we are
going to do that, then I think we need to document it somewhere to make
it harder to surprise developers.  Another alternative would be to add a
sequence number for each object type (cfm, bfd, lacp, stp?).  (If we did
that, then it would also solve the ordering dependency issue I mentioned
above.)

> ---
> v2: Fixed mutex safety in netdev_vport_changed()
>     Track STP changes
> 
> NB: I'm tracking STP changes in ofproto_port_set_state(), as the testsuite
> would fail when I tracked them in stp_set_port_state(). The latter makes more
> sense to me, but I clearly don't understand some interaction there because it
> makes STP stop working. I tried a few combinations in lib/stp.c, but I couldn't
> get any of them to work.

Hmm.  I agree that's odd.



More information about the dev mailing list