On Thu, Aug 6, 2015 at 10:20 PM, Haggai Eran <[email protected]> wrote: > On 08/06/2015 02:18 PM, Parav Pandit wrote: >> On Thu, Aug 6, 2015 at 4:30 PM, Haggai Eran <[email protected] >> <mailto:[email protected]>> wrote: >> >> On Wednesday, August 5, 2015 8:16 PM, Jason Gunthorpe >> [email protected] >> <mailto:[email protected]>> wrote: >> > On Wed, Aug 05, 2015 at 06:34:26PM +0300, Amir Vadai wrote: >> >> struct ib_uverbs_ex_query_device { >> >> __u32 comp_mask; >> >> + __u32 csum_caps; >> >> __u32 reserved; >> >> }; >> > >> > Uh no. >> This is the struct of the command, not the response. There is no >> need to extend it. The command is designed to always return as much >> information as possible, so the user space code doesn't need to pass >> anything for it to work. >> >> Even if you did want to extend it, you would need to replace the >> reserved word. The structs in this header file must be made in such >> way that they have the same size on 32-bit systems and on 64-bit >> systems (see the comment at the beginning of the header file). This >> is why the reserved word is there. >> >> > >> >> @@ -221,6 +222,7 @@ struct ib_uverbs_odp_caps { >> >> struct ib_uverbs_ex_query_device_resp { >> >> struct ib_uverbs_query_device_resp base; >> >> __u32 comp_mask; >> >> + __u32 csum_caps; >> >> __u32 response_length; >> >> struct ib_uverbs_odp_caps odp_caps; >> >> __u64 timestamp_mask; >> > >> > Also totally wrong. >> >> The response struct must maintain backward compatibility. You cannot >> change the order of the existing fields. The only valid way of >> extending it is at the end. Here too, you must make sure that the >> struct has the same size on 32-bit systems, so you would need to add >> a 32-bit reserved word at the end. >> >> Haggai >> >> As struct ib_uverbs_ex_query_device_resp captures extended capabilities, >> does it make sense to have few more reserved words defined as part of >> this patch? >> So that later on those reserved can be defined in future for additional >> features. >> This way for every new feature we dont need to bump structure size of >> ABI, not we need to define new set of ABI calls. >> Its hard to say how much more is sufficient, but was thinking of 8 >> 32-bit words. >> > > I don't see how increasing the size now would get you anything that > changing the returned response_length field wouldn't.
It won't. Eventually this code will have switch case for various different response length to support backward compatibility. I was trying to avoid adding such switch-case. Instead based on supported kernel version, it will fill up the information. > I'm not sure what > you consider an ABI change. Doesn't adding new meaning to reserved > fields count as a change? In any case, increasing the response length > doesn't require adding new calls. Yes, it doesn't. I don't see issue with response length increase, it solves it. I was considering a solution where we don't have to keep doing that. > The kernel code will agree to fill only the fields that fit in the buffer > provided by the user-space caller. > > Haggai -- 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
