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

[Chandran, Sugesh]

Regards
_Sugesh


> -----Original Message-----
> From: Roi Dayan [mailto:roid at mellanox.com]
> Sent: Wednesday, February 15, 2017 9:25 AM
> To: Chandran, Sugesh <sugesh.chandran at intel.com>; dev at openvswitch.org
> Cc: roid at mellanox.com; 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>
> Subject: Re: [PATCH ovs V3 10/25] netdev-tc-offloads: Add ufid to tc/netdev
> map
> 
> 
> 
> 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/
[Sugesh] thanks!
> 
> >> +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
> >