[ovs-dev] [PATCH ovs V2 05/21] dpif: Save added ports in a port map for netdev flow api use

Joe Stringer joe at ovn.org
Tue Jan 10 19:12:47 UTC 2017


On 10 January 2017 at 03:20, Paul Blakey <paulb at mellanox.com> wrote:
>
>
> On 05/01/2017 03:54, Joe Stringer wrote:
>>
>> On 25 December 2016 at 03:39, Paul Blakey <paulb at 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 at mellanox.com>
>>> Reviewed-by: Roi Dayan <roid at 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?


More information about the dev mailing list