On Wed, May 20, 2015 at 9:17 PM, Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx> wrote: > On Wed, May 20, 2015 at 07:27:15PM +0300, Matan Barak wrote: >> On Tue, May 19, 2015 at 9:06 PM, Jason Gunthorpe >> <jgunthorpe@xxxxxxxxxxxxxxxxxxxx> wrote: >> > On Tue, May 19, 2015 at 05:27:10PM +0300, Matan Barak wrote: >> > >> >> +#define __IB_ONLY >> > >> > What is this? >> > >> >> I annotated functions that should be called with IB link layer only as >> __IB_ONLY. I think it's make the code more readable and it could be >> verified by tools/scripts (like coccinelle) that we're calling those >> functions under an IB link layer if statement. > > Why not add: > > BUG_ON(!rdma_is_ib(...)) > > [or whatever the name ended up as] > > To the function prolog? > > Then it actually does something.. > I think static checks suffice here - they have a clear advantage by forcing users to call these functions under rdma_is_ib if statement. However, since static checks (still) aren't widely used by all developers, I can change that to BUG_ON like you suggested. >> >> + * @gid_type: The GID type to search for. >> >> + * @net: In RoCE, the namespace of the device. >> >> + * @if_index: In RoCE, the if_index of the device. Zero means ignore. >> >> * @port_num: The port number of the device where the GID value was found. >> >> * @index: The index into the cached GID table where the GID was found. This >> >> * parameter may be NULL. >> >> @@ -66,10 +82,36 @@ int ib_get_cached_gid(struct ib_device *device, >> >> */ >> >> int ib_find_cached_gid(struct ib_device *device, >> >> union ib_gid *gid, >> >> + enum ib_gid_type gid_type, >> >> + struct net *net, >> >> + int if_index, >> >> u8 *port_num, >> >> u16 *index); >> > >> > Why are we adding net namespace stuff here? We don't even have a >> > proposal for roce net namespaces. Same comment for all net adds. >> > >> >> In order to get a netdev, you need to have if_index and namespace. >> I prefer to introduce a correct API from the beginning than to assume >> we only use &init_net and changing the API afterwards. >> roce_gid_table and ib_cache should present a clean API no matter >> if their consumers use a custom namespace or init_net. > > No, this needs to be a cleanup series - get to the point where we are > sharing the gid table code between the drivers that need it. > > Don't change the APIs speculatively for rocev2 or namespaces, those > series can deal with it on their own. > The argument for removing the gid_type seems reasonable to me. However, I don't think we should be removing net. if_index should always come with net - passing only if_index makes roce_gid_table's API a bit broken. > To be more clear, the goal of this series should only be to get > patches 11,12,13,14 merged, and the minimum set of other patches to > get there. > > That means we don't need these API changes. We don't need ib_gid_attr. > Drop patch 8 and 9. > Patch 8 (the ndev part) is relevant. GID is now related to a ndev and we would like to expose this information to the user. In non rdma-cm applications, how would a user select the gid_index he wants? Patch 9 is used by 10, so if we don't drop 10 - we need 9. > I'm also not sure about patch 10, that looks like a functional > change? It should not be in a cleanup series. > We drop smac and vlan_id from path and av. Instead, it's taken from the netdev in roce_gid_table. That's a crucial part of the new architecture. It's not worth adding roce_gid_table and not taking the parameters based on its information. > Is 6 new functionality? Are drivers already handling bonding? > The mlx4 driver currently supports bonding - dropping this patch means functionality regression. > A cleanup series should have *no functional change*. > > So, it looks roughly like 7,8,9,10 can be dropped? > So if we sum everything up - do you agree with: Patch 7 - Change to BUG_ON, delete gid_type from API Patch 8 - delete gid_type from sysfs > Jason Thanks for looking at this patch. Matan -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
