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

Jesse Gross jesse at nicira.com
Mon Jan 10 18:05:06 UTC 2011


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.

> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 64b5193..b0dfb80 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -1389,37 +1389,27 @@ static int query_port(struct datapath *dp, struct odp_port __user *uport)
>        return put_port(vport, uport);
>  }
>
> -static int do_list_ports(struct datapath *dp, struct odp_port __user *uports,
> -                        int n_ports)
> +static int do_dump_port(struct datapath *dp, struct odp_vport_dump *dump)
>  {
> -       int idx = 0;
> -       if (n_ports) {
> -               struct vport *p;
> -
> -               list_for_each_entry_rcu (p, &dp->port_list, node) {
> -                       if (put_port(p, &uports[idx]))
> -                               return -EFAULT;
> -                       if (idx++ >= n_ports)
> -                               break;
> -               }
> +       u32 port_no;
> +
> +       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().

> +               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.

>  }
>
> -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.

> 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.




More information about the dev mailing list