[ovs-dev] [PATCH 11/16] tunneling: Add datapath GRE support.

Ben Pfaff blp at nicira.com
Thu Apr 15 17:37:15 UTC 2010


On Tue, Apr 13, 2010 at 10:41:13AM -0400, Jesse Gross wrote:
> Add a new vport type that implements GRE support inside of the
> datapath instead of relying on Linux devices.  This provides
> greater scalability, performance, and control.

It might be worth noting more specifically in the commit log the
problems that we saw with the netdev-based GRE.  If we ever want to
convince anyone upstream that the vport abstraction makes sense then we
will need details.

I would appreciate a brief comment on the GRE_F_IN_* constants and maybe
on the struct gre_port_config members also.  I don't know the design
well enough for their meanings to be obvious to me.

I think that new_config_rcu() could be simplified into a call to
kmemdup().

I think that a temporary could be eliminated from assign_config_rcu() by
calling call_rcu() before assigning the new configuration.

In add_port, I think that adding a tbl_count() or tbl_should_expand()
function would be more transparent than summing all of the *_ports
counts.

I doubt that the *_ports variables really need to be "atomic_t"s.  They
are only modified under the vport_lock.  They are only read outside that
lock to determine whether they are zero, in situations where races don't
matter.  I think that "unsigned int" would work just as well.  (Not that
atomic_t is a big deal, but it prompted me to look around to see if
anything tricky was going on.  Nothing was.)

I was surprised to find port_cmp() modifying its argument.  I didn't
expect that.  I guess that this is because the vport's "mutable" member
might change between the rcu_dereference in port_cmp() and any
opportunity the caller would have to dereference it.  (This could be
made more transparent by having the tbl code just return a bucket for
the caller to search, avoiding a callback function.)  I am kind of
surprised that "vport_gre"s matching criteria can change at all--I would
have guessed that a new vport would have to be created to replace the
old one.

I assume that find_port() is a fast-path?  I wonder whether there is a
way to avoid potentially four separate hashes and searches.  In practice
do you think it is likely that all four types of port matches will be in
use?

I skipped over all the code from check_ipv4_address() to gre_init()
because it looked very familiar.  I remember reviewing something like it
before.  If there are bits you want reviewed in detail, just let me
know.

I found myself wondering whether the minimum size of a "tbl" should be
smaller than a page.  I guess that there will often only be a few GRE
ports.

Is there anything that keeps the module loaded while gre ports exist?
It's not obvious to me.  (But 2.6.x kernels have some cleverness here
for which I've forgotten the details.)

Is gre_modify() only called when the lookup criteria change?  If not,
then it would seem better to avoid del_port()/add_port() if only, say,
the tos or ttl were to change, so that there is no time at which packets
would miss in the table during the change.

Also in gre_modify(), do we end up with an orphaned vport if del_port()
succeeds but add_port() fails?

It's pretty weird for tbl_remove() and thus del_port() to be able to
fail due to lack of memory.  Maybe we should fix that by allowing null
pointers in buckets.  Dunno.




More information about the dev mailing list