| Message ID | 1456215928-9305-8-git-send-email-leon@leon.nu (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Tue, Feb 23, 2016 at 10:25:27AM +0200, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@mellanox.com> > > This vector CALC feature allows different offloaded arithmetic > calculations of several vectors of equal length. > > In order to properly use this feature, the user space applications > need to know the supported properties, like operations, size and > maximal number of vectors. > > The properties exposed are: > * calc_matrix - If set, vector CALC matrix is supported. > * max_vector_count - Maximum number of vectors supported. > * max_chunk_size - Maximum chunk size supported. This kind of stuff should be in a kdoc not lost in a commit message. > * op_cap - Bit mask indicates which vector CALC operations are supported: > Bit 1: ADD operation > Bit 2: MAX operation > Bit 3: AND operation > Bit 4: OR operation > Bit 5: XOR operation > Bit 6: MIN operation > Bit 7: SWAP_ENDIANNESS operation This should be constants in the uAPI header. A commit message is not documentation. Can you defend why this proprietary extension is being shoved into the common uapi and not dumped in the vendor area? Is an IBTA standardization forthcoming? Have you collaborated with other vendors to agree on this API? I really agree with Christoph Hellwig, this continuous churn on the common api for non-standard features is really bad. We need to have a higher bar for acceptance, which is something stronger than one vendor implementing something in their hardware. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jason, >> The properties exposed are: >> * calc_matrix - If set, vector CALC matrix is supported. >> * max_vector_count - Maximum number of vectors supported. >> * max_chunk_size - Maximum chunk size supported. > > This kind of stuff should be in a kdoc not lost in a commit message. I agree. >> * op_cap - Bit mask indicates which vector CALC operations are supported: >> Bit 1: ADD operation >> Bit 2: MAX operation >> Bit 3: AND operation >> Bit 4: OR operation >> Bit 5: XOR operation >> Bit 6: MIN operation >> Bit 7: SWAP_ENDIANNESS operation > > This should be constants in the uAPI header. > > A commit message is not documentation. Agree here too. > Can you defend why this proprietary extension is being shoved into the > common uapi and not dumped in the vendor area? Is an IBTA > standardization forthcoming? Have you collaborated with other vendors > to agree on this API? > > I really agree with Christoph Hellwig, this continuous churn on the > common api for non-standard features is really bad. We need to have a > higher bar for acceptance, which is something stronger than one vendor > implementing something in their hardware. I also agree with Christoph, however I don't know if we should require IBTA standardization for each feature we add to our ibverbs. This feature introduces application specific offloads, there are no wire protocol implications. It's a question of view I guess, my view is that we want to get our verbs closer to what applications want rather then restricting it to be a messaging API, we just need to make sure that the API fits the application requirements. I think that it's the community's job to decide if this is a) useful and b) exposed and implemented correctly. This patchset is exactly a collaboration with other vendors to agree on the API. And I honestly don't understand the "one vendor" complaint. It's really hard to make progress this way? Yes, one vendor introduces a feature, it's always the case. Do we need to wait for others to do it too for it to make upstream? If a feature is useful and exposed correctly there is nothing stopping other vendors to do it too (given that the API is not tied to a vendor implementation). Generally speaking, I'm confident that all the smart people on Linux-rdma can converge on suitable APIs that are actually useful, maintainable and don't impose a single vendor implementation. Leon, Or, Matan and others are more than willing to receive input from the community. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 02, 2016 at 07:07:58PM +0200, Sagi Grimberg wrote: > And I honestly don't understand the "one vendor" complaint. It's really > hard to make progress this way? Yes, one vendor introduces a feature, > it's always the case. Do we need to wait for others to do it too for it > to make upstream? If a feature is useful and exposed correctly there is > nothing stopping other vendors to do it too (given that the API is > not tied to a vendor implementation). I don't necessarily mean other vendors have to implement it, just sign off on it. We have the IBTA as a multi-vendor body that has historically defined the verbs API. It would be appropriate if verbs changes were ack'd by their SW-WG. The OFA has talked about a 'verbs working group' (what happened to that Liran?) that could also evolve into an appropriate place to get such a sign off. It may be a more inclusive/neutral place these days with so many non-IB verbs implementations. > Generally speaking, I'm confident that all the smart people on > Linux-rdma can converge on suitable APIs that are actually useful, The thing is, I don't think the right people are necessarily reading this list. There are now many vendors of verbs hardware out there, do you really think posting to the list is enough to catch all of their attention? My concern is changes to verbs are very rarely a software only change - there is almost always a hardware component and within the industry cross-vendor hardware specifications are typically negotiated by trade associations not the Linux Kernel mailing list. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jason, >> And I honestly don't understand the "one vendor" complaint. It's really >> hard to make progress this way? Yes, one vendor introduces a feature, >> it's always the case. Do we need to wait for others to do it too for it >> to make upstream? If a feature is useful and exposed correctly there is >> nothing stopping other vendors to do it too (given that the API is >> not tied to a vendor implementation). > > I don't necessarily mean other vendors have to implement it, just sign > off on it. > > We have the IBTA as a multi-vendor body that has historically defined > the verbs API. It would be appropriate if verbs changes were ack'd by > their SW-WG. I think it did, I'll let Liran answer on that. > The OFA has talked about a 'verbs working group' (what happened to > that Liran?) that could also evolve into an appropriate place to get > such a sign off. It may be a more inclusive/neutral place these days > with so many non-IB verbs implementations. I'll let Liran comment on this too, but I know for a fact that Liran is setting these meetings on a weekly basis and the vast majority of the features Mellanox send out are introduced there. I joined a few times to talk about some of the kernel API changes we did lately and the upcoming "erasure coding offload" feature in the storage space... >> Generally speaking, I'm confident that all the smart people on >> Linux-rdma can converge on suitable APIs that are actually useful, > > The thing is, I don't think the right people are necessarily reading > this list. There are now many vendors of verbs hardware out there, do > you really think posting to the list is enough to catch all of their > attention? Actually I disagree, our verbs API is evolving and I don't see a better place to shape and build it other than Linux-rdma. A vendor that cares about RDMA verbs API should listen to this list, participate in the discussions (also the API related ones) and help in bug fixes and maintenance. That's the best way vendors can promote their cause (user-space or kernel). > My concern is changes to verbs are very rarely a software only change > - there is almost always a hardware component and within the industry > cross-vendor hardware specifications are typically negotiated by trade > associations not the Linux Kernel mailing list. That's true, but almost all user-space management stuff are handled in the kernel leaving user-space to either ask uverbs or the HW device for a service. Are you suggesting that new offload features that are proposed would also introduce a SW implementation that can run on any device? That's not a terrible idea, but it would be a lot of extra work and might bloat our verbs library. What do others think? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 23, 2016 at 11:44:00AM -0700, Jason Gunthorpe wrote: > On Tue, Feb 23, 2016 at 10:25:27AM +0200, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@mellanox.com> > > > > This vector CALC feature allows different offloaded arithmetic > > calculations of several vectors of equal length. > > > > In order to properly use this feature, the user space applications > > need to know the supported properties, like operations, size and > > maximal number of vectors. > > > > The properties exposed are: > > * calc_matrix - If set, vector CALC matrix is supported. > > * max_vector_count - Maximum number of vectors supported. > > * max_chunk_size - Maximum chunk size supported. > > This kind of stuff should be in a kdoc not lost in a commit message. > > > * op_cap - Bit mask indicates which vector CALC operations are supported: > > Bit 1: ADD operation > > Bit 2: MAX operation > > Bit 3: AND operation > > Bit 4: OR operation > > Bit 5: XOR operation > > Bit 6: MIN operation > > Bit 7: SWAP_ENDIANNESS operation > > This should be constants in the uAPI header. > > A commit message is not documentation. Sorry for not actively participating in this discussion, I was traveling with terrible internet access. I'll be more than happy to send the version with your concerns fixed. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- > owner@vger.kernel.org] On Behalf Of Jason Gunthorpe > > The OFA has talked about a 'verbs working group' (what happened to that > Liran?) that could also evolve into an appropriate place to get such a sign off. It > may be a more inclusive/neutral place these days with so many non-IB verbs > implementations. > The Verbs working group has been meeting twice a month since its inception to discuss such emerging features and new concepts. --Liran -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c index 3a3f573..3618ca5 100644 --- a/drivers/infiniband/core/uverbs_cmd.c +++ b/drivers/infiniband/core/uverbs_cmd.c @@ -3641,6 +3641,15 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, resp.device_cap_flags2 = upper_32_bits(attr.device_cap_flags); resp.response_length += sizeof(resp.device_cap_flags2); + if (ucore->outlen < resp.response_length + sizeof(resp.calc_caps)) + goto end; + + resp.calc_caps.calc_matrix = attr.calc_caps.calc_matrix; + resp.calc_caps.max_vector_count = attr.calc_caps.max_vector_count; + resp.calc_caps.max_chunk_size = attr.calc_caps.max_chunk_size; + resp.calc_caps.op_cap = attr.calc_caps.op_cap; + resp.response_length += sizeof(resp.calc_caps); + end: err = ib_copy_to_udata(ucore, &resp, resp.response_length); return err; diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 51aabf8..1f74ebb 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -258,6 +258,24 @@ struct ib_odp_caps { } per_transport_caps; }; +enum ib_calc_operations { + IB_CALC_NONE, + IB_CALC_ADD, + IB_CALC_MAX, + IB_CALC_AND, + IB_CALC_OR, + IB_CALC_XOR, + IB_CALC_MIN, + IB_CALC_SWAP_ENDIANNESS +}; + +struct ib_calc_caps { + u32 calc_matrix; + u32 max_vector_count; + u32 max_chunk_size; + u32 op_cap; +}; + enum ib_cq_creation_flags { IB_CQ_FLAGS_TIMESTAMP_COMPLETION = 1 << 0, IB_CQ_FLAGS_IGNORE_OVERRUN = 1 << 1, @@ -315,6 +333,7 @@ struct ib_device_attr { struct ib_odp_caps odp_caps; uint64_t timestamp_mask; uint64_t hca_core_clock; /* in KHZ */ + struct ib_calc_caps calc_caps; }; enum ib_mtu { diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h index e024c82..c88d8e2 100644 --- a/include/uapi/rdma/ib_user_verbs.h +++ b/include/uapi/rdma/ib_user_verbs.h @@ -219,6 +219,13 @@ struct ib_uverbs_odp_caps { __u32 reserved; }; +struct ib_uverbs_calc_caps { + __u32 calc_matrix; + __u32 max_vector_count; + __u32 max_chunk_size; + __u32 op_cap; +}; + struct ib_uverbs_ex_query_device_resp { struct ib_uverbs_query_device_resp base; __u32 comp_mask; @@ -231,6 +238,7 @@ struct ib_uverbs_ex_query_device_resp { * by __u32 variable. Need to increase this field */ __u64 device_cap_flags2; + struct ib_uverbs_calc_caps calc_caps; }; struct ib_uverbs_query_port {