On Fri, Nov 10, 2017 at 02:15:29PM -0500, Doug Ledford wrote:
> 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.
I'll fix commit log, rebase and resubmit, but it is not clear to me the
benefits of changing kernel version of modify_cq and all their users
to extended version. I think that we better convert them once they
actually will require it.
Thanks
Attachment:
signature.asc
Description: PGP signature
