[ovs-dev] [netlink v3 08/16] datapath: Change listing ports to use an iterator concept.

[Ben Pfaff]

On Mon, Jan 10, 2011 at 01:05:06PM -0500, Jesse Gross wrote:
> On Wed, Dec 29, 2010 at 7:56 PM, Ben Pfaff <blp at nicira.com> wrote:
> > One of the goals for Open vSwitch is to decouple kernel and userspace
> > software, so that either one can be upgraded or rolled back independent of
> > the other. ?To do this in full generality, it must be possible to add new
> > features to the kernel vport layer without changing userspace software. ?In
> > turn, that means that the odp_port structure must become variable-length.
> > This does not, however, fit in well with the ODP_PORT_LIST ioctl in its
> > current form, because that would require userspace to know how much space
> > to allocate for each port in advance, or to allocate as much space as
> > could possibly be needed. ?Neither choice is very attractive.
> >
> > This commit prepares for a different solution, by replacing ODP_PORT_LIST
> > by a new ioctl ODP_VPORT_DUMP that retrieves information about a single
> > vport from the datapath on each call. ?It is much cleaner to allocate the
> > maximum amount of space for a single vport than to do so for possibly a
> > large number of vports.
> >
> > It would be faster to retrieve a number of vports in batch instead of just
> > one at a time, but that will naturally happen later when the kernel
> > datapath interface is changed to use Netlink, so this patch does not bother
> > with it.
> 
> The same general comment about this type of iterator interface between
> userspace/kernel.

The problem is much less severe with ports, to the point that I wasn't
worried about it at all, because the position in the table is
well-defined.  The flow table gets resized, and adding and deleting
flows causes shifting around within buckets, but ports always keep their
port numbers.

> > + ? ? ? for (port_no = dump->port_no; port_no < DP_MAX_PORTS; port_no++) {
> > + ? ? ? ? ? ? ? struct vport *vport = dp->ports[port_no];
> 
> This should use get_vport_protected().

OK, fixed.

> > + ? ? ? ? ? ? ? if (vport)
> > + ? ? ? ? ? ? ? ? ? ? ? return put_port(vport, (struct odp_port __force __user*)dump->port);
> 
> This is a little inconsistent with the dump flows iterator, which
> automatically advances itself.  It's particularly odd because this
> function is so close to query_port().  I would either expect
> query_port() to be used for this to act like a true iterator.

In my view, the flow iterator is the oddball.  It is really hard for
userspace to say where to continue iteration in the flow table, because
there is no stable, monotonic identifier for a flow.  But it's easy for
userspace to say "start dumping from port 0" at the beginning or "start
dumping from port X+1" if it is already looking at port X.

> > ?}
> >
> > -static int list_ports(struct datapath *dp, struct odp_portvec __user *upv)
> > +static int dump_port(struct datapath *dp, struct odp_vport_dump __user *udump)
> > ?{
> > - ? ? ? struct odp_portvec pv;
> > - ? ? ? int retval;
> > + ? ? ? struct odp_vport_dump dump;
> >
> > - ? ? ? if (copy_from_user(&pv, upv, sizeof pv))
> > + ? ? ? if (copy_from_user(&dump, udump, sizeof dump))
> 
> sizeof should have parentheses.

OK, fixed.

> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 96a24fd..0cb3b9e 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -592,38 +592,40 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
> [...]
> > ? ? LIST_FOR_EACH (br, node, &all_bridges) {
> > - ? ? ? ?struct odp_port *dpif_ports;
> > - ? ? ? ?size_t n_dpif_ports;
> > + ? ? ? ?struct dpif_port_info {
> > + ? ? ? ? ? ?uint32_t port_no;
> > + ? ? ? ? ? ?char *type;
> > + ? ? ? ?};
> 
> I find it a little weird to be defining a new structure like this.
> Why not just copy the struct odp_port?  Also, this is almost identical
> to the struct dpif_port that gets defined in later commit and I would
> expect it to get replaced by that but it doesn't look like it is.

Much of what goes on in this series in userspace (which will become even
more apparent as I continue to post patches) is to separate the
interface that dpif presents to OVS from the interface that the Linux
datapath presents to userspace.  This is why I did not want to use
odp_port here.

It is a good idea to use dpif_port, though, so I made that change,
renaming dpif_port_info to dpif_port for this commit and later deleting
the definition in favor of the one in lib/dpif.h.