[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