On Wed, May 20, 2020 at 5:32 PM Yamin Friedman <yaminf@xxxxxxxxxxxx> wrote:
>
>
> On 5/20/2020 1:50 PM, Devesh Sharma wrote:
> > On Wed, May 20, 2020 at 2:53 PM Yamin Friedman <yaminf@xxxxxxxxxxxx> wrote:
> >>
> >> On 5/20/2020 9:19 AM, Devesh Sharma wrote:
> >>>> +
> >>>> +static int ib_alloc_cqs(struct ib_device *dev, int nr_cqes,
> >>>> + enum ib_poll_context poll_ctx)
> >>>> +{
> >>>> + LIST_HEAD(tmp_list);
> >>>> + struct ib_cq *cq;
> >>>> + unsigned long flags;
> >>>> + int nr_cqs, ret, i;
> >>>> +
> >>>> + /*
> >>>> + * Allocated at least as many CQEs as requested, and otherwise
> >>>> + * a reasonable batch size so that we can share CQs between
> >>>> + * multiple users instead of allocating a larger number of CQs.
> >>>> + */
> >>>> + nr_cqes = min(dev->attrs.max_cqe, max(nr_cqes, IB_MAX_SHARED_CQ_SZ));
> >>>> + nr_cqs = min_t(int, dev->num_comp_vectors, num_online_cpus());
> >>> No WARN() or return with failure as pointed by Leon and me. Has
> >>> anything else changes elsewhere?
> >> Hey Devesh,
> >>
> >> I am not sure what you are referring to, could you please clarify?
> >>
> > I thought on V2 Leon gave a comment "how this will work if
> > dev->num_comp_vectors" is 0.
> > there I had suggested to fail the pool creation and issue a
> > WARN_ONCE() or something.
>
> I understood his comment to be regarding if the comp_vector itself is 0.
> There should not be any issue with that case.
>
> As far as I am aware there must be a non-zero amount of comp_vectors for
> the ib_dev otherwise we will not be able to get any indication for cqes.
> I don't see any reason to add a special check here.
>
Okay, maybe a WARN_ONCE() would be useful from a debug point of view.
Otherwise for a given buggy driver, things may not be obvious and the
user may still think that the pool was created successfully, but
traffic will not move.
Add it if you see a value in this point of view otherwise:
Reviewed-by: Devesh Sharma <devesh.sharma@xxxxxxxxxxxx>
Thanks
> Thanks
>
> >>>> + for (i = 0; i < nr_cqs; i++) {
> >>>> + cq = ib_alloc_cq(dev, NULL, nr_cqes, i, poll_ctx);
> >>>> + if (IS_ERR(cq)) {
> >>>> + ret = PTR_ERR(cq);
> >>>> + goto out_free_cqs;
> >>>> + }
> >>>> + cq->shared = true;
> >>>> + list_add_tail(&cq->pool_entry, &tmp_list);
> >>>> + }
> >>>> +
> >>>> + spin_lock_irqsave(&dev->cq_pools_lock, flags);
> >>>> + list_splice(&tmp_list, &dev->cq_pools[poll_ctx - 1]);
> >>>> + spin_unlock_irqrestore(&dev->cq_pools_lock, flags);
> >>>> +
> >>>> + return 0;
> >>>> +
> >>>> +out_free_cqs:
> >>>> + list_for_each_entry(cq, &tmp_list, pool_entry) {
> >>>> + cq->shared = false;
> >>>> + ib_free_cq(cq);
> >>>> + }
> >>>> + return ret;
> >>>> +}
> >>>> +
> >>>>
