On 10 January 2017 at 03:20, Paul Blakey <pa...@mellanox.com> wrote:
>
>
> On 05/01/2017 03:54, Joe Stringer wrote:
>>
>> On 25 December 2016 at 03:39, Paul Blakey <pa...@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 <pa...@mellanox.com>
>>> Reviewed-by: Roi Dayan <r...@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?
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
