On Thu, May 28, 2015 at 8:07 PM, Jason Gunthorpe
<[email protected]> wrote:
> On Thu, May 28, 2015 at 07:34:36PM +0300, Or Gerlitz wrote:
>
>> As for the RoCE GID table itself, adding in properly net-devices in their
>> native Linux kernel form, namely with if_index and name-space -- seems to me
>> the correct way to go.
>
> Well, no, it is goofy. Callers with a path have a if_index/net pair.
> Several other callers have an actual struct netdevice.
>
> The first thing roce_gid_table_find_gid_by_port does is convert the
> if_index/net pair back to a netdevice. This tells me the API is wrong,
> callers that already have a netdevice should just pass it in, and path
> is one that should be special cased to search.
>
> But I don't even want to talk about that for a clean up series.
>
ib_find_cached_gid is used in cm_init_av_by_path. ndev is necessary
in order to find the correct GID in the GID table, as otherwise if two upper
devices has the same IP (hence GID), we might not found the bounded
device correctly.
Therefore, I suggest modifying ib_find_cached_gid (add netdevice),
In addition, vendors need to get the MAC of the ndev that relates to the GID
in order to output a correct Ethernet header. This is currently done though
ib_get_cached_gid. Without adding gid_attr, you get a buggy implementation
that upper devices with different HW addresses would all map to the same MAC.
> I want to know that the behavior of ib_find_cached_gid does not
> change. If it hasn't changed then we don't need to change call
> sites to pass in the optional netdev, because it must work as it does
> today. Otherwise the cleanup is broken.
>
Prior to those changes, the vendor resolved the MAC address twice.
The current patchset *cleans* this up - instead of resolving all over again
we map a GID entry from ndev.
This conforms to a cleanup series as *no new functionality is added*.
Modified API doesn't always imply new functionality but sometimes better design.
> So, my specific advice is:
>
> - roce_gid_table_find_gid_by_port should accept a netdevice or null
I agree
> - Always pass null for the netdevice for all call sites
I think we should stick with the path->net and path->if_index.
They are used to indicate the bounded device of the cma.
This device is then used in the ib_find_cached_gid.
> - Do not change the ib_* APIs during the clean up.
>
I think we should keep the modified
(a) ib_get_cached_gid (output gid_attr)
(b) ib_find_cached_gid (input ndev)
(c) ib_find_cached_gid_by_port (input ndev)
(d) ib_query_gid (output gid_attr)
> Jason
Thanks for your review and looking for your comments regarding this.
Matan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
