On 7 April 2017 at 06:12, Roi Dayan <[email protected]> wrote:
> From: Paul Blakey <[email protected]>
>
> To use netdev flow offloading api, dpifs needs to iterate over
> added ports. This addition inserts the added dpif ports in a hash map,
> The map will also be used to translate dpif ports to netdevs.
>
> Signed-off-by: Paul Blakey <[email protected]>
> Reviewed-by: Roi Dayan <[email protected]>
> Reviewed-by: Simon Horman <[email protected]>
> ---
> lib/dpif.c | 25 ++++++++++++
> lib/dpif.h | 2 +
> lib/netdev.c | 123
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> lib/netdev.h | 8 ++++
> 4 files changed, 158 insertions(+)
>
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 1760de8..d45f21d 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -355,7 +355,22 @@ do_open(const char *name, const char *type, bool create,
> struct dpif **dpifp)
> error = registered_class->dpif_class->open(registered_class->dpif_class,
> name, create, &dpif);
> if (!error) {
> + struct dpif_port_dump port_dump;
> + struct dpif_port dpif_port;
> +
> ovs_assert(dpif->dpif_class == registered_class->dpif_class);
> +
> + DPIF_PORT_FOR_EACH(&dpif_port, &port_dump, dpif) {
> + struct netdev *netdev;
> + int err = netdev_open(dpif_port.name, dpif_port.type, &netdev);
> +
> + if (!err) {
> + netdev_hmap_port_add(netdev, DPIF_HMAP_KEY(dpif),
> &dpif_port);
> + netdev_close(netdev);
> + } else {
> + VLOG_WARN("could not open netdev %s type %s", name, type);
> + }
> + }
> } else {
> dp_class_unref(registered_class);
> }
> @@ -548,6 +563,14 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev,
> odp_port_t *port_nop)
> if (!error) {
> VLOG_DBG_RL(&dpmsg_rl, "%s: added %s as port %"PRIu32,
> dpif_name(dpif), netdev_name, port_no);
> +
> + /* temp dpif_port, will be cloned in netdev_hmap_port_add */
> + struct dpif_port dpif_port;
> +
> + dpif_port.type = CONST_CAST(char *, netdev_get_type(netdev));
> + dpif_port.name = CONST_CAST(char *, netdev_name);
> + dpif_port.port_no = port_no;
> + netdev_hmap_port_add(netdev, DPIF_HMAP_KEY(dpif), &dpif_port);
> } else {
> VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s",
> dpif_name(dpif), netdev_name, ovs_strerror(error));
> @@ -572,6 +595,8 @@ dpif_port_del(struct dpif *dpif, odp_port_t port_no)
> if (!error) {
> VLOG_DBG_RL(&dpmsg_rl, "%s: port_del(%"PRIu32")",
> dpif_name(dpif), port_no);
> +
> + netdev_hmap_port_del(port_no, DPIF_HMAP_KEY(dpif));
> } else {
> log_operation(dpif, "port_del", error);
> }
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 5d49b11..81376c0 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -401,6 +401,8 @@
> extern "C" {
> #endif
>
> +#define DPIF_HMAP_KEY(x) ((x)->dpif_class)
> +
> struct dpif;
> struct dpif_class;
> struct dpif_flow;
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 6bde14b..a4de0a9 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -2104,6 +2104,129 @@ netdev_init_flow_api(struct netdev *netdev)
> : EOPNOTSUPP);
> }
>
> +/* Protects below port hashmaps. */
> +static struct ovs_mutex netdev_hmap_mutex = OVS_MUTEX_INITIALIZER;
> +
> +static struct hmap port_to_netdev OVS_GUARDED_BY(netdev_hmap_mutex)
> + = HMAP_INITIALIZER(&port_to_netdev);
> +static struct hmap ifindex_to_port OVS_GUARDED_BY(netdev_hmap_mutex)
> + = HMAP_INITIALIZER(&ifindex_to_port);
> +
> +struct port_to_netdev_data {
> + struct hmap_node node;
> + struct netdev *netdev;
> + struct dpif_port dpif_port;
> + const void *obj;
> +};
> +
> +struct ifindex_to_port_data {
> + struct hmap_node node;
> + int ifindex;
> + odp_port_t port;
> +};
> +
> +static struct port_to_netdev_data *
> +netdev_hmap_port_lookup(odp_port_t port_no, const void *obj)
> +{
> + size_t hash = hash_int(odp_to_u32(port_no), hash_pointer(obj, 0));
> + struct port_to_netdev_data *data;
> +
> + HMAP_FOR_EACH_WITH_HASH(data, node, hash, &port_to_netdev) {
> + if (data->obj == obj && data->dpif_port.port_no == port_no) {
> + return data;
> + }
> + }
> + return NULL;
> +}
> +
> +int
> +netdev_hmap_port_add(struct netdev *netdev, const void *obj,
> + struct dpif_port *dpif_port)
> +{
> + size_t hash = hash_int(odp_to_u32(dpif_port->port_no),
> + hash_pointer(obj, 0));
> + struct port_to_netdev_data *data = xzalloc(sizeof *data);
> + struct ifindex_to_port_data *ifidx = xzalloc(sizeof *ifidx);
> +
> + ovs_mutex_lock(&netdev_hmap_mutex);
> + if (netdev_hmap_port_lookup(dpif_port->port_no, obj)) {
> + ovs_mutex_unlock(&netdev_hmap_mutex);
> + return EEXIST;
> + }
> +
> + data->netdev = netdev_ref(netdev);
> + data->obj = obj;
> + dpif_port_clone(&data->dpif_port, dpif_port);
> +
> + ifidx->ifindex = netdev_get_ifindex(netdev);
I think that this function can return -EOPNOTSUPP, which is not handled.
> + ifidx->port = dpif_port->port_no;
> +
> + hmap_insert(&port_to_netdev, &data->node, hash);
> + hmap_insert(&ifindex_to_port, &ifidx->node, ifidx->ifindex);
> + ovs_mutex_unlock(&netdev_hmap_mutex);
> +
> + netdev_init_flow_api(netdev);
> +
> + return 0;
> +}
> +
> +struct netdev *
> +netdev_hmap_port_get(odp_port_t port_no, const void *obj)
> +{
> + struct port_to_netdev_data *data;
> + struct netdev *ret = NULL;
> +
> + ovs_mutex_lock(&netdev_hmap_mutex);
> + data = netdev_hmap_port_lookup(port_no, obj);
> + if (data) {
> + ret = netdev_ref(data->netdev);
> + }
> + ovs_mutex_unlock(&netdev_hmap_mutex);
> +
> + return ret;
> +}
> +
> +odp_port_t
> +netdev_hmap_port_get_byifidx(int ifindex)
> +{
> + struct ifindex_to_port_data *data;
> + odp_port_t ret = 0;
> +
> + ovs_mutex_lock(&netdev_hmap_mutex);
> + HMAP_FOR_EACH_WITH_HASH(data, node, ifindex, &ifindex_to_port) {
> + if (data->ifindex == ifindex) {
> + ret = data->port;
> + break;
> + }
> + }
> + ovs_mutex_unlock(&netdev_hmap_mutex);
> +
> + return ret;
> +}
> +
> +int
> +netdev_hmap_port_del(odp_port_t port_no, const void *obj)
> +{
> + struct port_to_netdev_data *data;
> + int ret = ENOENT;
> +
> + ovs_mutex_lock(&netdev_hmap_mutex);
> +
> + data = netdev_hmap_port_lookup(port_no, obj);
> +
> + if (data) {
> + dpif_port_destroy(&data->dpif_port);
> + netdev_close(data->netdev); /* unref and possibly close */
> + hmap_remove(&port_to_netdev, &data->node);
> + free(data);
> + ret = 0;
> + }
> +
> + ovs_mutex_unlock(&netdev_hmap_mutex);
> +
> + return ret;
> +}
> +
> bool netdev_flow_api_enabled = false;
>
> #ifdef __linux__
> diff --git a/lib/netdev.h b/lib/netdev.h
> index e2d1799..f494ea3 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -181,6 +181,14 @@ int netdev_init_flow_api(struct netdev *);
> extern bool netdev_flow_api_enabled;
> void netdev_set_flow_api_enabled(const struct smap *ovs_other_config);
>
> +struct dpif_port;
> +int netdev_hmap_port_add(struct netdev *, const void *obj, struct dpif_port
> *);
> +struct netdev *netdev_hmap_port_get(odp_port_t port, const void *obj);
> +int netdev_hmap_port_del(odp_port_t port, const void *obj);
> +struct netdev_flow_dump **netdev_ports_flow_dumps_create(const void *obj,
> + int *ports);
> +odp_port_t netdev_hmap_port_get_byifidx(int ifindex);
It seems like 'obj' is trying to go half way on object-orientation in
these functions. 'obj' doesn't describe very well the semantics of how
that variable is passed and used. I feel like it'd be easier to follow
what's going on with these functions if there were a 'struct
netdev_ports' which provided the container and locking for the map. It
could be attached to the dpif, then these functions could each receive
one of these 'struct netdev_ports *' structures that would more
clearly put a box around what 'obj' is trying to contain. Then we also
get some nice compiler type help which we don't get by passing void
pointers around.
The naming could be a bit more consistent for the add/get/del/dump -
common prefix netdev_ports_* perhaps?
netdev_ports_insert()
netdev_ports_lookup()
netdev_ports_remove()
netdev_ports_create_flow_dump()
The last function name feels a bit squashed, if you're trying to keep
the names short then maybe it'd be easier to read as something like
netdev_get_port_by_ifindex() or netdev_ifindex_to_odp_port()?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
