| Message ID | 1482665989-791-6-git-send-email-paulb@mellanox.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 25 December 2016 at 03:39, Paul Blakey <paulb@mellanox.com> wrote: > 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 <paulb@mellanox.com> > Reviewed-by: Roi Dayan <roid@mellanox.com> > --- > lib/dpif.c | 25 +++++++++++ > lib/netdev.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > lib/netdev.h | 14 +++++++ > 3 files changed, 172 insertions(+) > > diff --git a/lib/dpif.c b/lib/dpif.c > index 53958c5..c67ea92 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -352,7 +352,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->dpif_class, &dpif_port); > + netdev_close(netdev); > + } else { > + VLOG_WARN("could not open netdev %s type %s", name, type); > + } > + } > } else { > dp_class_unref(registered_class); > } > @@ -545,6 +560,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->dpif_class, &dpif_port); > } else { > VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s", > dpif_name(dpif), netdev_name, ovs_strerror(error)); > @@ -569,6 +592,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->dpif_class->type); > } else { > log_operation(dpif, "port_del", error); > } > diff --git a/lib/netdev.c b/lib/netdev.c > index b289166..2630802 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -2080,6 +2080,139 @@ netdev_init_flow_api(struct netdev *netdev) > : EOPNOTSUPP); > } > > +static struct hmap port_to_netdev = HMAP_INITIALIZER(&port_to_netdev); > +static struct hmap ifindex_to_port = 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; > +}; > + > +int > +netdev_hmap_port_add(struct netdev *netdev, const void *obj, > + struct dpif_port *dpif_port) > +{ > + size_t hash = hash_int(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); > + > + netdev_hmap_port_del(dpif_port->port_no, obj); > + > + data->netdev = netdev_ref(netdev); > + data->obj = obj; > + dpif_port_clone(&data->dpif_port, dpif_port); > + > + ifidx->ifindex = netdev_get_ifindex(netdev); > + ifidx->port = dpif_port->port_no; > + > + hmap_insert(&port_to_netdev, &data->node, hash); > + hmap_insert(&ifindex_to_port, &ifidx->node, ifidx->ifindex); > + > + return 0; > +} > + > +struct netdev * > +netdev_hmap_port_get(odp_port_t port_no, const void *obj) > +{ > + size_t hash = hash_int(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) { > + break; > + } > + } > + > + if (data) { > + netdev_ref(data->netdev); > + return data->netdev; > + } > + > + return 0; > +} > + > +odp_port_t > +netdev_hmap_port_get_byifidx(int ifindex) netdev_hmap_get_port_by_ifindex? > +{ > + struct ifindex_to_port_data *data; > + > + HMAP_FOR_EACH_WITH_HASH(data, node, ifindex, &ifindex_to_port) { > + if (data->ifindex == ifindex) { > + return data->port; > + } > + } > + > + return 0; > +} > + > +int > +netdev_hmap_port_del(odp_port_t port_no, const void *obj) > +{ > + size_t hash = hash_int(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) { > + break; > + } > + } > + > + 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); > + return 0; > + } > + > + return ENOENT; > +} > + > +int > +netdev_hmap_port_get_list(const void *obj, struct ovs_list *list) > +{ > + struct port_to_netdev_data *data; > + int count = 0; > + > + ovs_list_init(list); > + > + HMAP_FOR_EACH(data, node, &port_to_netdev) { > + if (data->obj == obj) { > + struct netdev_list_element *element = xzalloc(sizeof *element); > + > + element->netdev = netdev_ref(data->netdev); > + element->port_no = data->dpif_port.port_no; > + ovs_list_insert(list, &element->node); > + count++; > + } > + } > + > + return count; > +} > + > +void > +netdev_port_list_del(struct ovs_list *list) > +{ > + struct netdev_list_element *element; > + > + if (!list) { > + return; > + } > + > + LIST_FOR_EACH_POP(element, node, list) { > + netdev_close(element->netdev); > + free(element); > + } > +} > + It seems that all of the users of these list assemble/delete functions do: netdev_hmap_port_get_list(...) <iterate list to get what they actually want> netdev_port_list_del(...) Perhaps the functions that call these should actually live in lib/netdev.c, and just get exactly what they want directly from the hmap instead of allocating list elements, iterating the netdev list, and freeing the elements (in addition to the iteration they actually want to do). > bool netdev_flow_api_enabled = false; Different patch, but should this be static?
On 05/01/2017 03:54, Joe Stringer wrote: > On 25 December 2016 at 03:39, Paul Blakey <paulb@mellanox.com> wrote: >> 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 <paulb@mellanox.com> >> Reviewed-by: Roi Dayan <roid@mellanox.com> >> --- >> lib/dpif.c | 25 +++++++++++ >> lib/netdev.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> lib/netdev.h | 14 +++++++ >> 3 files changed, 172 insertions(+) >> >> diff --git a/lib/dpif.c b/lib/dpif.c >> index 53958c5..c67ea92 100644 >> --- a/lib/dpif.c >> +++ b/lib/dpif.c >> @@ -352,7 +352,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->dpif_class, &dpif_port); >> + netdev_close(netdev); >> + } else { >> + VLOG_WARN("could not open netdev %s type %s", name, type); >> + } >> + } >> } else { >> dp_class_unref(registered_class); >> } >> @@ -545,6 +560,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->dpif_class, &dpif_port); >> } else { >> VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s", >> dpif_name(dpif), netdev_name, ovs_strerror(error)); >> @@ -569,6 +592,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->dpif_class->type); >> } else { >> log_operation(dpif, "port_del", error); >> } >> diff --git a/lib/netdev.c b/lib/netdev.c >> index b289166..2630802 100644 >> --- a/lib/netdev.c >> +++ b/lib/netdev.c >> @@ -2080,6 +2080,139 @@ netdev_init_flow_api(struct netdev *netdev) >> : EOPNOTSUPP); >> } >> >> +static struct hmap port_to_netdev = HMAP_INITIALIZER(&port_to_netdev); >> +static struct hmap ifindex_to_port = 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; >> +}; >> + >> +int >> +netdev_hmap_port_add(struct netdev *netdev, const void *obj, >> + struct dpif_port *dpif_port) >> +{ >> + size_t hash = hash_int(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); >> + >> + netdev_hmap_port_del(dpif_port->port_no, obj); >> + >> + data->netdev = netdev_ref(netdev); >> + data->obj = obj; >> + dpif_port_clone(&data->dpif_port, dpif_port); >> + >> + ifidx->ifindex = netdev_get_ifindex(netdev); >> + ifidx->port = dpif_port->port_no; >> + >> + hmap_insert(&port_to_netdev, &data->node, hash); >> + hmap_insert(&ifindex_to_port, &ifidx->node, ifidx->ifindex); >> + >> + return 0; >> +} >> + >> +struct netdev * >> +netdev_hmap_port_get(odp_port_t port_no, const void *obj) >> +{ >> + size_t hash = hash_int(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) { >> + break; >> + } >> + } >> + >> + if (data) { >> + netdev_ref(data->netdev); >> + return data->netdev; >> + } >> + >> + return 0; >> +} >> + >> +odp_port_t >> +netdev_hmap_port_get_byifidx(int ifindex) > netdev_hmap_get_port_by_ifindex? > >> +{ >> + struct ifindex_to_port_data *data; >> + >> + HMAP_FOR_EACH_WITH_HASH(data, node, ifindex, &ifindex_to_port) { >> + if (data->ifindex == ifindex) { >> + return data->port; >> + } >> + } >> + >> + return 0; >> +} >> + >> +int >> +netdev_hmap_port_del(odp_port_t port_no, const void *obj) >> +{ >> + size_t hash = hash_int(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) { >> + break; >> + } >> + } >> + >> + 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); >> + return 0; >> + } >> + >> + return ENOENT; >> +} >> + >> +int >> +netdev_hmap_port_get_list(const void *obj, struct ovs_list *list) >> +{ >> + struct port_to_netdev_data *data; >> + int count = 0; >> + >> + ovs_list_init(list); >> + >> + HMAP_FOR_EACH(data, node, &port_to_netdev) { >> + if (data->obj == obj) { >> + struct netdev_list_element *element = xzalloc(sizeof *element); >> + >> + element->netdev = netdev_ref(data->netdev); >> + element->port_no = data->dpif_port.port_no; >> + ovs_list_insert(list, &element->node); >> + count++; >> + } >> + } >> + >> + return count; >> +} >> + >> +void >> +netdev_port_list_del(struct ovs_list *list) >> +{ >> + struct netdev_list_element *element; >> + >> + if (!list) { >> + return; >> + } >> + >> + LIST_FOR_EACH_POP(element, node, list) { >> + netdev_close(element->netdev); >> + free(element); >> + } >> +} >> + > It seems that all of the users of these list assemble/delete functions do: > > netdev_hmap_port_get_list(...) > <iterate list to get what they actually want> > netdev_port_list_del(...) > > Perhaps the functions that call these should actually live in > lib/netdev.c, and just get exactly what they want directly from the > hmap instead of allocating list elements, iterating the netdev list, > and freeing the elements (in addition to the iteration they actually > want to do). Doesn't that break encapsulation a bit? This is used to iterate over the ports/netdevs and: 1) flush their offloaded rules 2) start a dump for offloaded rules That will introduce all that is required for dumping/flushing to lib/netdev. I suggest we use a higher order function instead, that will save the allocations and freeing. /* Netdev dumping. */ struct netdev_list_element { struct ovs_list node; struct netdev *netdev; odp_port_t port_no; }; /* return true to stop iterating, element is next on the list */ typedef bool (*portIterator)(netdev_list_element *element); /* pseudo code: */ void netdev_hmap_port_for_each(const void *obj, portIterator func) { struct netdev_list_element; for each in netdev hmap { fill element with next if (func(element)) break; } } >> bool netdev_flow_api_enabled = false; > Different patch, but should this be static? Yes, thanks.
On 10 January 2017 at 03:20, Paul Blakey <paulb@mellanox.com> wrote: > > > On 05/01/2017 03:54, Joe Stringer wrote: >> >> On 25 December 2016 at 03:39, Paul Blakey <paulb@mellanox.com> wrote: >>> >>> 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 <paulb@mellanox.com> >>> Reviewed-by: Roi Dayan <roid@mellanox.com> >>> --- >>> lib/dpif.c | 25 +++++++++++ >>> lib/netdev.c | 133 >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> lib/netdev.h | 14 +++++++ >>> 3 files changed, 172 insertions(+) >>> >>> diff --git a/lib/dpif.c b/lib/dpif.c >>> index 53958c5..c67ea92 100644 >>> --- a/lib/dpif.c >>> +++ b/lib/dpif.c >>> @@ -352,7 +352,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->dpif_class, >>> &dpif_port); >>> + netdev_close(netdev); >>> + } else { >>> + VLOG_WARN("could not open netdev %s type %s", name, >>> type); >>> + } >>> + } >>> } else { >>> dp_class_unref(registered_class); >>> } >>> @@ -545,6 +560,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->dpif_class, &dpif_port); >>> } else { >>> VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s", >>> dpif_name(dpif), netdev_name, >>> ovs_strerror(error)); >>> @@ -569,6 +592,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->dpif_class->type); >>> } else { >>> log_operation(dpif, "port_del", error); >>> } >>> diff --git a/lib/netdev.c b/lib/netdev.c >>> index b289166..2630802 100644 >>> --- a/lib/netdev.c >>> +++ b/lib/netdev.c >>> @@ -2080,6 +2080,139 @@ netdev_init_flow_api(struct netdev *netdev) >>> : EOPNOTSUPP); >>> } >>> >>> +static struct hmap port_to_netdev = HMAP_INITIALIZER(&port_to_netdev); >>> +static struct hmap ifindex_to_port = 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; >>> +}; >>> + >>> +int >>> +netdev_hmap_port_add(struct netdev *netdev, const void *obj, >>> + struct dpif_port *dpif_port) >>> +{ >>> + size_t hash = hash_int(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); >>> + >>> + netdev_hmap_port_del(dpif_port->port_no, obj); >>> + >>> + data->netdev = netdev_ref(netdev); >>> + data->obj = obj; >>> + dpif_port_clone(&data->dpif_port, dpif_port); >>> + >>> + ifidx->ifindex = netdev_get_ifindex(netdev); >>> + ifidx->port = dpif_port->port_no; >>> + >>> + hmap_insert(&port_to_netdev, &data->node, hash); >>> + hmap_insert(&ifindex_to_port, &ifidx->node, ifidx->ifindex); >>> + >>> + return 0; >>> +} >>> + >>> +struct netdev * >>> +netdev_hmap_port_get(odp_port_t port_no, const void *obj) >>> +{ >>> + size_t hash = hash_int(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) >>> { >>> + break; >>> + } >>> + } >>> + >>> + if (data) { >>> + netdev_ref(data->netdev); >>> + return data->netdev; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +odp_port_t >>> +netdev_hmap_port_get_byifidx(int ifindex) >> >> netdev_hmap_get_port_by_ifindex? >> >>> +{ >>> + struct ifindex_to_port_data *data; >>> + >>> + HMAP_FOR_EACH_WITH_HASH(data, node, ifindex, &ifindex_to_port) { >>> + if (data->ifindex == ifindex) { >>> + return data->port; >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +int >>> +netdev_hmap_port_del(odp_port_t port_no, const void *obj) >>> +{ >>> + size_t hash = hash_int(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) >>> { >>> + break; >>> + } >>> + } >>> + >>> + 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); >>> + return 0; >>> + } >>> + >>> + return ENOENT; >>> +} >>> + >>> +int >>> +netdev_hmap_port_get_list(const void *obj, struct ovs_list *list) >>> +{ >>> + struct port_to_netdev_data *data; >>> + int count = 0; >>> + >>> + ovs_list_init(list); >>> + >>> + HMAP_FOR_EACH(data, node, &port_to_netdev) { >>> + if (data->obj == obj) { >>> + struct netdev_list_element *element = xzalloc(sizeof >>> *element); >>> + >>> + element->netdev = netdev_ref(data->netdev); >>> + element->port_no = data->dpif_port.port_no; >>> + ovs_list_insert(list, &element->node); >>> + count++; >>> + } >>> + } >>> + >>> + return count; >>> +} >>> + >>> +void >>> +netdev_port_list_del(struct ovs_list *list) >>> +{ >>> + struct netdev_list_element *element; >>> + >>> + if (!list) { >>> + return; >>> + } >>> + >>> + LIST_FOR_EACH_POP(element, node, list) { >>> + netdev_close(element->netdev); >>> + free(element); >>> + } >>> +} >>> + >> >> It seems that all of the users of these list assemble/delete functions do: >> >> netdev_hmap_port_get_list(...) >> <iterate list to get what they actually want> >> netdev_port_list_del(...) >> >> Perhaps the functions that call these should actually live in >> lib/netdev.c, and just get exactly what they want directly from the >> hmap instead of allocating list elements, iterating the netdev list, >> and freeing the elements (in addition to the iteration they actually >> want to do). > > Doesn't that break encapsulation a bit? > This is used to iterate over the ports/netdevs and: > 1) flush their offloaded rules > 2) start a dump for offloaded rules Hmm. dpif_netlink_flow_flush() does: <get ports list> netdev_flow_flush() (This already exists in netdev.c) <delete ports list> start_netdev_dump() does: <get ports list> <for each element, take a couple of pieces of netdev and store them into another local list/array> <delete ports list> parse_flow_get() does: <get ports list> netdev_flow_get() (This already exists in netdev.c) <delete ports list> parse_flow_del() does: <get ports list> netdev_flow_del() (This already exists in netdev.c) <delete ports list> All I'm suggesting is that, for example: netdev_hmap_port_get_list(dpif_->dpif_class, &port_list); LIST_FOR_EACH(element, node, &port_list) { netdev_flow_flush(element->netdev); } netdev_port_list_del(&port_list); Turns into: netdev_ports_flow_flush(dpif_->dpif_class); I don't think this is much different from the encapsulation that already exists around the ports hmap in netdev. > That will introduce all that is required for dumping/flushing to lib/netdev. > I suggest we use a higher order function instead, that will save the > allocations and freeing. > > /* Netdev dumping. */ > struct netdev_list_element { > struct ovs_list node; > struct netdev *netdev; > odp_port_t port_no; > }; > > /* return true to stop iterating, element is next on the list */ > typedef bool (*portIterator)(netdev_list_element *element); > > /* pseudo code: */ > > void netdev_hmap_port_for_each(const void *obj, portIterator func) { > struct netdev_list_element; > for each in netdev hmap { > fill element with next > if (func(element)) break; > } > } I'm on the fence about this, it's more generic but it's also a little more complicated. Not sure it's necessary at this stage. One more thing - what's protecting the netdev hmap against concurrent adds/deletes and safe iteration?
diff --git a/lib/dpif.c b/lib/dpif.c index 53958c5..c67ea92 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -352,7 +352,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->dpif_class, &dpif_port); + netdev_close(netdev); + } else { + VLOG_WARN("could not open netdev %s type %s", name, type); + } + } } else { dp_class_unref(registered_class); } @@ -545,6 +560,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->dpif_class, &dpif_port); } else { VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s", dpif_name(dpif), netdev_name, ovs_strerror(error)); @@ -569,6 +592,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->dpif_class->type); } else { log_operation(dpif, "port_del", error); } diff --git a/lib/netdev.c b/lib/netdev.c index b289166..2630802 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -2080,6 +2080,139 @@ netdev_init_flow_api(struct netdev *netdev) : EOPNOTSUPP); } +static struct hmap port_to_netdev = HMAP_INITIALIZER(&port_to_netdev); +static struct hmap ifindex_to_port = 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; +}; + +int +netdev_hmap_port_add(struct netdev *netdev, const void *obj, + struct dpif_port *dpif_port) +{ + size_t hash = hash_int(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); + + netdev_hmap_port_del(dpif_port->port_no, obj); + + data->netdev = netdev_ref(netdev); + data->obj = obj; + dpif_port_clone(&data->dpif_port, dpif_port); + + ifidx->ifindex = netdev_get_ifindex(netdev); + ifidx->port = dpif_port->port_no; + + hmap_insert(&port_to_netdev, &data->node, hash); + hmap_insert(&ifindex_to_port, &ifidx->node, ifidx->ifindex); + + return 0; +} + +struct netdev * +netdev_hmap_port_get(odp_port_t port_no, const void *obj) +{ + size_t hash = hash_int(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) { + break; + } + } + + if (data) { + netdev_ref(data->netdev); + return data->netdev; + } + + return 0; +} + +odp_port_t +netdev_hmap_port_get_byifidx(int ifindex) +{ + struct ifindex_to_port_data *data; + + HMAP_FOR_EACH_WITH_HASH(data, node, ifindex, &ifindex_to_port) { + if (data->ifindex == ifindex) { + return data->port; + } + } + + return 0; +} + +int +netdev_hmap_port_del(odp_port_t port_no, const void *obj) +{ + size_t hash = hash_int(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) { + break; + } + } + + 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); + return 0; + } + + return ENOENT; +} + +int +netdev_hmap_port_get_list(const void *obj, struct ovs_list *list) +{ + struct port_to_netdev_data *data; + int count = 0; + + ovs_list_init(list); + + HMAP_FOR_EACH(data, node, &port_to_netdev) { + if (data->obj == obj) { + struct netdev_list_element *element = xzalloc(sizeof *element); + + element->netdev = netdev_ref(data->netdev); + element->port_no = data->dpif_port.port_no; + ovs_list_insert(list, &element->node); + count++; + } + } + + return count; +} + +void +netdev_port_list_del(struct ovs_list *list) +{ + struct netdev_list_element *element; + + if (!list) { + return; + } + + LIST_FOR_EACH_POP(element, node, list) { + netdev_close(element->netdev); + free(element); + } +} + bool netdev_flow_api_enabled = false; void diff --git a/lib/netdev.h b/lib/netdev.h index 5be67ea..4ef134f 100644 --- a/lib/netdev.h +++ b/lib/netdev.h @@ -171,6 +171,20 @@ int netdev_init_flow_api(struct netdev *); extern bool netdev_flow_api_enabled; void netdev_set_flow_api_enabled(bool flow_api_enabled); +/* Netdev dumping. */ +struct netdev_list_element { + struct ovs_list node; + struct netdev *netdev; + odp_port_t port_no; +}; +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); +int netdev_hmap_port_get_list(const void *obj, struct ovs_list *); +void netdev_port_list_del(struct ovs_list *list); +odp_port_t netdev_hmap_port_get_byifidx(int ifindex); + /* native tunnel APIs */ /* Structure to pass parameters required to build a tunnel header. */ struct netdev_tnl_build_header_params {