On Tue, Apr 17, 2012 at 03:46:00PM -0700, Ethan Jackson wrote:
> I'm having a bit of trouble understanding this patch.
> 
> It seems to me like it continues to do accounting when removing
> facets, what it's actually doing is avoiding learning when removing
> them.  I may be misinterpreting though.  Just to make sure I'm
> understanding: this is an optimization because it avoid the redundant
> call to xlate_actions() in facet_learn()?

Yeah.  It's a bad commit message.  I replaced it with:

    ofproto-dpif: Don't do an extra flow translation when removing facets.
    
    When we remove a facet, it seems superfluous to translate the flow
    once more just to update the MAC learning table, since we know that
    it was done within a second or so anyway (e.g. when the flow stats
    were last updated).
    
    Signed-off-by: Ben Pfaff <b...@nicira.com>

> Why remove "facet->accounted_bytes = facet->byte_count" from
> facet_account(), every caller seems to do it manually now.

Both facet_learn() and facet_account() use 'accounted_bytes', so if
either one of them updates it then the order in which one must call
them is artificially forced.  I thought that this was a better way.

> I think I'm just a bit confused, it may be worth expanding the commit
> message a bit.

I hope that the above is sufficiently better.

I'm going to push this series soon.

Thank you for all the reviews.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to