[ovs-dev] [PATCH ovs V3 10/25] netdev-tc-offloads: Add ufid to tc/netdev map

[Roi Dayan]

On 14/02/2017 01:54, Chandran, Sugesh wrote:
>
>
> Regards
> _Sugesh
>
>
>> -----Original Message-----
>> From: Roi Dayan [mailto:roid at mellanox.com]
>> Sent: Wednesday, February 8, 2017 3:29 PM
>> To: dev at openvswitch.org
>> Cc: Paul Blakey <paulb at mellanox.com>; Or Gerlitz
>> <ogerlitz at mellanox.com>; Hadar Hen Zion <hadarh at mellanox.com>; Shahar
>> Klein <shahark at mellanox.com>; Mark Bloch <markb at mellanox.com>; Rony
>> Efraim <ronye at mellanox.com>; Fastabend, John R
>> <john.r.fastabend at intel.com>; Joe Stringer <joe at ovn.org>; Andy
>> Gospodarek <andy at greyhouse.net>; Lance Richardson
>> <lrichard at redhat.com>; Marcelo Ricardo Leitner <mleitner at redhat.com>;
>> Simon Horman <simon.horman at netronome.com>; Jiri Pirko
>> <jiri at mellanox.com>; Chandran, Sugesh <sugesh.chandran at intel.com>
>> Subject: [PATCH ovs V3 10/25] netdev-tc-offloads: Add ufid to tc/netdev
>> map
>
> .....
>> +
> [Sugesh] Good to have comment on each functions explaining about the return values and parameters?

ok

>> +static bool
>> +del_ufid_tc_mapping(const ovs_u128 *ufid) {
>> +    size_t hash = hash_bytes(ufid, sizeof *ufid, 0);
>> +    struct ufid_to_tc_data *data;
>> +
>> +    ovs_mutex_lock(&ufid_lock);
>> +    HMAP_FOR_EACH_WITH_HASH(data, node, hash, &ufid_to_tc) {
>> +        if (ovs_u128_equals(*ufid, data->ufid)) {
>> +            break;
>> +        }
>> +    }
>> +    if (data) {
>> +        hmap_remove(&ufid_to_tc, &data->node);
>> +        ovs_mutex_unlock(&ufid_lock);
>> +        netdev_close(data->netdev);
>> +        free(data);
>> +        return true;
>> +    }
>> +    ovs_mutex_unlock(&ufid_lock);
>> +    return false;
>> +}
>> +
>> +static ovs_u128 *
>> +find_ufid(int prio, int handle, struct netdev *netdev) {
>> +    int ifindex = netdev_get_ifindex(netdev);
>> +    struct ufid_to_tc_data *data;
>> +
>> +    ovs_mutex_lock(&ufid_lock);
>> +    HMAP_FOR_EACH(data, node, &ufid_to_tc) {
>> +        if (data->prio == prio && data->handle == handle
>> +            && netdev_get_ifindex(data->netdev) == ifindex) {
>> +            break;
>> +        }
>> +    }
>> +    ovs_mutex_unlock(&ufid_lock);
>> +    if (data) {
>> +        return &data->ufid;
>> +    }
>> +    return NULL;
>> +}
>> +
>> +static int
>> +get_ufid_tc_mapping(const ovs_u128 *ufid, int *prio, struct netdev
>> +**netdev) {
>> +    size_t hash = hash_bytes(ufid, sizeof *ufid, 0);
>> +    struct ufid_to_tc_data *data;
>> +
>> +    ovs_mutex_lock(&ufid_lock);
>> +    HMAP_FOR_EACH_WITH_HASH(data, node, hash, &ufid_to_tc) {
>> +        if (ovs_u128_equals(*ufid, data->ufid)) {
>> +            break;
>> +        }
>> +    }
>> +    ovs_mutex_unlock(&ufid_lock);
>> +    if (data) {
>> +        if (prio) {
>> +            *prio = data->prio;
>> +        }
>> +        if (netdev) {
>> +            *netdev = netdev_ref(data->netdev);
>> +        }
>> +        return data->handle;
>> +    }
>> +    return 0;
>> +}
>> +
> [Sugesh] I assume its add +replace than just add? May be its good to add comment or changing the function name accordingly.

ok
I'll also reorder a little here. add/del/get/find/

>> +static bool
>> +add_ufid_tc_mapping(const ovs_u128 *ufid, int prio, int handle,
>> +                    struct netdev *netdev) {
>> +    size_t hash = hash_bytes(ufid, sizeof *ufid, 0);
>> +    bool replace = del_ufid_tc_mapping(ufid);
>> +    struct ufid_to_tc_data *new_data = xzalloc(sizeof *new_data);
>> +
>> +    new_data->ufid = *ufid;
>> +    new_data->prio = prio;
>> +    new_data->handle = handle;
>> +    new_data->netdev = netdev_ref(netdev);
>> +
>> +    ovs_mutex_lock(&ufid_lock);
>> +    hmap_insert(&ufid_to_tc, &new_data->node, hash);
>> +    ovs_mutex_unlock(&ufid_lock);
>> +
>> +    return replace;
>> +}
>> +
>>  int
>>  netdev_tc_flow_flush(struct netdev *netdev)  {
>> --
>> 2.7.4
>