Re: [PATCH rdma-next v1 1/6] IB/uverbs: Allow CQ moderation with modify CQ

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2417 bytes --]

On Mon, 2017-10-30 at 17:28 +0200, Leon Romanovsky wrote:
> On Mon, Oct 30, 2017 at 08:48:07AM -0600, Jason Gunthorpe wrote:
> > On Sun, Oct 29, 2017 at 08:28:08PM +0200, Leon Romanovsky wrote:
> > 
> > > > > +int ib_uverbs_ex_modify_cq(struct ib_uverbs_file *file,
> > > > > +			   struct ib_device *ib_dev,
> > > > > +			   struct ib_udata *ucore,
> > > > > +			   struct ib_udata *uhw)
> > > > 
> > > > Is this really a good idea?
> > > > 
> > > > Why not ib_uverbs_set_cq_moderation ?
> > > 
> > > It follows already existed ib_modify_cq(), see commit 2dd571622787
> > > ("IB/core: Add support for modify CQ")
> > 
> > And that function should have been called set_cq_moderation:
> > 
> > + * ib_modify_cq - Modifies moderation params of the CQ
> > + * @cq: The CQ to modify.
> > + * @cq_count: number of CQEs that will trigger an event
> > + * @cq_period: max period of time in usec before triggering an event
> > + *
> > + */
> > +int ib_modify_cq(struct ib_cq *cq, u16 cq_count, u16 cq_period);
> 
> I see it differently, this is extendable version of modify_cq, which is
> going to benefit all other users who will decide to extend it.

If it's the extendable version, then it should have just passed the attr
struct (or equivalent), it shouldn't have spelled out the moderation
parameters in the function signature.  So, either we need to change the
signature of ib_modify_cq to a generic, extendable signature, or we need
to change the name as Jason points out so we match name and parameter
signature in the same spirit.

Also, as you point out, need to update the log message to not use
cookie.

Let's please make this consistent before merging.  BTW, because so much
of the rest of the API uses things like modify_qp with an attr struct
and a single entry point, I'm leaning towards following that here for
the sake of API consistency.  Although I can see Jason's point about
having simpler entry points for the API, I don't think it buys us
anything in the RDMA space because we already have to be aware of and
use the monolithic single entry point API style.  I'm more inclined to
think a single API style is better than mixed API styles, just like
mixed coding styles is bad.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]