On Sun, Dec 31, 2017 at 1:45 PM, Yishai Hadas <yishaih@xxxxxxxxxxxxxxxxxx> wrote: > On 12/29/2017 5:23 PM, Or Gerlitz wrote: >> >> validate_driver_qp() deals @ 95% of it's contents with DC but has no >> DC in it's name, also it does assignment which goes beyond validation. >> > > OK, will rename it to be prepare_driver_qp(). sounds better, not it is very DC specific now, so you are happy with the way it is now w.r.t future extensions? > The function name follows the input QP type (i.e. IB_QPT_DRIVER) and what it > does on. Specifically, its purpose is to prepare the input data (ucmd, > init_attr) to be used later on. Similar name and logic is used as part of RQ > creation as part of this file. (i.e. prepare_user_rq()). >> There are bunch of things to consider fixing here. The patch spaghettizes >> the driver code w.r.t where the qp type is stored, in the ib qp > It doesn't make sense to overwrite the input QP type (i.e. IB_QPT_DRIVER) > which was supplied by the core layer to some internal sub-type. why? >> or the mlx5 qp > The sub type is from type enum ib_qp_type (MLX5_IB_QPT_DCI > <->IB_QPT_RESERVED3) and not an internal mlx5 PRM QP type. > However, to make the code more clearer and accurate, we'll change as part of > V2 'u32 driver_qp_type' to be 'enum ib_qp_type sub_qp_type' and clean some > redundant checks when applicable. (e.g. in mlx5_ib_query_qp()). just do your best to have the driver checks **one** field for the qp type for all QPs, otherwise I expect nice pile of future bugs around this corner. >> Also you added a "is qp ok" self function which AFAIK uses a different >> practice >> from the IB core copy, did you consider to refactor the core code and >> allow >> a re-use from the driver? > > > Yes, we considered that, however, as this QP type has some specific driver > behavior we found it better to stay with that specific logic for DCI QP in > the driver code, no reason to refactor the core layer as of that. As part of > V2 will rename 'mlx5_ib_modify_qp_is_ok()' to be 'modify_dc_qp_is_ok()' to > better represent the specific use case around. So what you are saying is the core logic being good for all QP types X all drivers X all transports and only mlx5/DC needs this different treatment? interesting, so what is the source for this misalignment? >> did you consider to use a similar practice >> for your "is ok" function? > Yes, however, we found it more clearer and simple to use current code for > the specific DCI case instead of introducing some general mapping table > without any real use. if you can map DC to the general framework, it means you can actually refactor the core helper and call it from mlx5, sounds better to me. -- 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
