Re: [PATCH] RDMA/core: reduce IB_POLL_BATCH constant

Re: [PATCH] RDMA/core: reduce IB_POLL_BATCH constant

[Jason Gunthorpe]

On Thu, Feb 22, 2018 at 05:39:09PM +0200, Sagi Grimberg wrote:
> 
> >>The only reason why I added this array on-stack was to allow consumers
> >>that did not use ib_alloc_cq api to call it, but that seems like a
> >>wrong decision when thinking it over again (as probably these users
> >>did not set the wr_cqe correctly).
> >>
> >>How about we make ib_process_cq_direct use the cq wc array and add
> >>a WARN_ON statement (and fail it gracefully) if the caller used this
> >>API without calling ib_alloc_cq?
> >
> >but we tried to avoid cuncurrent access to cq->wc.
> 
> Not sure its a valid use-case. But if there is a compelling
> reason to keep it as is, then we can do smaller on-stack
> array.

Did we come to a conclusion what to do here?

Jason

Re: [PATCH] RDMA/core: reduce IB_POLL_BATCH constant

[Max Gurtovoy]

On 2/28/2018 12:09 AM, Jason Gunthorpe wrote:
> On Thu, Feb 22, 2018 at 05:39:09PM +0200, Sagi Grimberg wrote:
>>
>>>> The only reason why I added this array on-stack was to allow consumers
>>>> that did not use ib_alloc_cq api to call it, but that seems like a
>>>> wrong decision when thinking it over again (as probably these users
>>>> did not set the wr_cqe correctly).
>>>>
>>>> How about we make ib_process_cq_direct use the cq wc array and add
>>>> a WARN_ON statement (and fail it gracefully) if the caller used this
>>>> API without calling ib_alloc_cq?
>>>
>>> but we tried to avoid cuncurrent access to cq->wc.
>>
>> Not sure its a valid use-case. But if there is a compelling
>> reason to keep it as is, then we can do smaller on-stack
>> array.
> 
> Did we come to a conclusion what to do here?

guys,
what do you think about the following solution (untested):


diff --git a/drivers/infiniband/core/cq.c b/drivers/infiniband/core/cq.c
index bc79ca8..59d2835 100644
--- a/drivers/infiniband/core/cq.c
+++ b/drivers/infiniband/core/cq.c
@@ -17,6 +17,7 @@

  /* # of WCs to poll for with a single call to ib_poll_cq */
  #define IB_POLL_BATCH                  16
+#define IB_POLL_BATCH_DIRECT           8

  /* # of WCs to iterate over before yielding */
  #define IB_POLL_BUDGET_IRQ             256
@@ -25,17 +26,25 @@
  #define IB_POLL_FLAGS \
         (IB_CQ_NEXT_COMP | IB_CQ_REPORT_MISSED_EVENTS)

-static int __ib_process_cq(struct ib_cq *cq, int budget, struct ib_wc 
*poll_wc)
+static int __ib_process_cq(struct ib_cq *cq, int budget, struct ib_wc 
*poll_wc,
+                          int batch)
  {
-       int i, n, completed = 0;
-       struct ib_wc *wcs = poll_wc ? : cq->wc;
-
+       int i, n, ib_poll_batch, completed = 0;
+       struct ib_wc *wcs;
+
+       if (poll_wc) {
+               wcs = poll_wc;
+               ib_poll_batch = batch;
+       } else {
+               wcs = cq->wc;
+               ib_poll_batch = IB_POLL_BATCH;
+       }
         /*
          * budget might be (-1) if the caller does not
          * want to bound this call, thus we need unsigned
          * minimum here.
          */
-       while ((n = ib_poll_cq(cq, min_t(u32, IB_POLL_BATCH,
+       while ((n = ib_poll_cq(cq, min_t(u32, ib_poll_batch,
                         budget - completed), wcs)) > 0) {
                 for (i = 0; i < n; i++) {
                         struct ib_wc *wc = &wcs[i];
@@ -48,7 +57,7 @@ static int __ib_process_cq(struct ib_cq *cq, int 
budget, struct ib_wc *poll_wc)

                 completed += n;

-               if (n != IB_POLL_BATCH ||
+               if (n != ib_poll_batch ||
                     (budget != -1 && completed >= budget))
                         break;
         }
@@ -72,9 +81,9 @@ static int __ib_process_cq(struct ib_cq *cq, int 
budget, struct ib_wc *poll_wc)
   */
  int ib_process_cq_direct(struct ib_cq *cq, int budget)
  {
-       struct ib_wc wcs[IB_POLL_BATCH];
+       struct ib_wc wcs[IB_POLL_BATCH_DIRECT];

-       return __ib_process_cq(cq, budget, wcs);
+       return __ib_process_cq(cq, budget, wcs, IB_POLL_BATCH_DIRECT);
  }
  EXPORT_SYMBOL(ib_process_cq_direct);

@@ -88,7 +97,7 @@ static int ib_poll_handler(struct irq_poll *iop, int 
budget)
         struct ib_cq *cq = container_of(iop, struct ib_cq, iop);
         int completed;

-       completed = __ib_process_cq(cq, budget, NULL);
+       completed = __ib_process_cq(cq, budget, NULL, 0);
         if (completed < budget) {
                 irq_poll_complete(&cq->iop);
                 if (ib_req_notify_cq(cq, IB_POLL_FLAGS) > 0)
@@ -108,7 +117,7 @@ static void ib_cq_poll_work(struct work_struct *work)
         struct ib_cq *cq = container_of(work, struct ib_cq, work);
         int completed;

-       completed = __ib_process_cq(cq, IB_POLL_BUDGET_WORKQUEUE, NULL);
+       completed = __ib_process_cq(cq, IB_POLL_BUDGET_WORKQUEUE, NULL, 0);
         if (completed >= IB_POLL_BUDGET_WORKQUEUE ||
             ib_req_notify_cq(cq, IB_POLL_FLAGS) > 0)
                 queue_work(ib_comp_wq, &cq->work);



> 
> Jason
> 

Re: [PATCH] RDMA/core: reduce IB_POLL_BATCH constant

[Bart Van Assche]

On 02/27/18 14:15, Max Gurtovoy wrote:
> -static int __ib_process_cq(struct ib_cq *cq, int budget, struct ib_wc 
> *poll_wc)
> +static int __ib_process_cq(struct ib_cq *cq, int budget, struct ib_wc 
> *poll_wc,
> +                          int batch)
>   {
> -       int i, n, completed = 0;
> -       struct ib_wc *wcs = poll_wc ? : cq->wc;
> +       int i, n, ib_poll_batch, completed = 0;
> +       struct ib_wc *wcs;
> +
> +       if (poll_wc) {
> +               wcs = poll_wc;
> +               ib_poll_batch = batch;
> +       } else {
> +               wcs = cq->wc;
> +               ib_poll_batch = IB_POLL_BATCH;
> +       }

Since this code has to be touched I think that we can use this 
opportunity to get rid of the "poll_wc ? : cq->wc" conditional and 
instead use what the caller passes. That will require to update all 
__ib_process_cq(..., ..., NULL) calls. I also propose to let the caller 
pass ib_poll_batch instead of figuring it out in this function. 
Otherwise the approach of this patch looks fine to me.

Thanks,

Bart.

Re: [PATCH] RDMA/core: reduce IB_POLL_BATCH constant

[Max Gurtovoy]

On 2/28/2018 2:21 AM, Bart Van Assche wrote:
> On 02/27/18 14:15, Max Gurtovoy wrote:
>> -static int __ib_process_cq(struct ib_cq *cq, int budget, struct ib_wc 
>> *poll_wc)
>> +static int __ib_process_cq(struct ib_cq *cq, int budget, struct ib_wc 
>> *poll_wc,
>> +                          int batch)
>>   {
>> -       int i, n, completed = 0;
>> -       struct ib_wc *wcs = poll_wc ? : cq->wc;
>> +       int i, n, ib_poll_batch, completed = 0;
>> +       struct ib_wc *wcs;
>> +
>> +       if (poll_wc) {
>> +               wcs = poll_wc;
>> +               ib_poll_batch = batch;
>> +       } else {
>> +               wcs = cq->wc;
>> +               ib_poll_batch = IB_POLL_BATCH;
>> +       }
> 
> Since this code has to be touched I think that we can use this 
> opportunity to get rid of the "poll_wc ? : cq->wc" conditional and 
> instead use what the caller passes. That will require to update all 
> __ib_process_cq(..., ..., NULL) calls. I also propose to let the caller 
> pass ib_poll_batch instead of figuring it out in this function. 
> Otherwise the approach of this patch looks fine to me.

Thanks Bart.
I'll make these changes and submit.

> 
> Thanks,
> 
> Bart.

-Max.

Re: [PATCH] RDMA/core: reduce IB_POLL_BATCH constant

[Doug Ledford]

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

On Wed, 2018-02-28 at 11:50 +0200, Max Gurtovoy wrote:
> 
> On 2/28/2018 2:21 AM, Bart Van Assche wrote:
> > On 02/27/18 14:15, Max Gurtovoy wrote:
> > > -static int __ib_process_cq(struct ib_cq *cq, int budget, struct ib_wc 
> > > *poll_wc)
> > > +static int __ib_process_cq(struct ib_cq *cq, int budget, struct ib_wc 
> > > *poll_wc,
> > > +                          int batch)
> > >   {
> > > -       int i, n, completed = 0;
> > > -       struct ib_wc *wcs = poll_wc ? : cq->wc;
> > > +       int i, n, ib_poll_batch, completed = 0;
> > > +       struct ib_wc *wcs;
> > > +
> > > +       if (poll_wc) {
> > > +               wcs = poll_wc;
> > > +               ib_poll_batch = batch;
> > > +       } else {
> > > +               wcs = cq->wc;
> > > +               ib_poll_batch = IB_POLL_BATCH;
> > > +       }
> > 
> > Since this code has to be touched I think that we can use this 
> > opportunity to get rid of the "poll_wc ? : cq->wc" conditional and 
> > instead use what the caller passes. That will require to update all 
> > __ib_process_cq(..., ..., NULL) calls. I also propose to let the caller 
> > pass ib_poll_batch instead of figuring it out in this function. 
> > Otherwise the approach of this patch looks fine to me.
> 
> Thanks Bart.
> I'll make these changes and submit.

That sounds reasonable to me too, thanks for reworking and resubmitting.

-- 
Doug Ledford <dledford@redhat.com>
    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 --]

Re: [PATCH] RDMA/core: reduce IB_POLL_BATCH constant

[Max Gurtovoy]

On 2/28/2018 8:55 PM, Doug Ledford wrote:
> On Wed, 2018-02-28 at 11:50 +0200, Max Gurtovoy wrote:
>>
>> On 2/28/2018 2:21 AM, Bart Van Assche wrote:
>>> On 02/27/18 14:15, Max Gurtovoy wrote:
>>>> -static int __ib_process_cq(struct ib_cq *cq, int budget, struct ib_wc
>>>> *poll_wc)
>>>> +static int __ib_process_cq(struct ib_cq *cq, int budget, struct ib_wc
>>>> *poll_wc,
>>>> +                          int batch)
>>>>    {
>>>> -       int i, n, completed = 0;
>>>> -       struct ib_wc *wcs = poll_wc ? : cq->wc;
>>>> +       int i, n, ib_poll_batch, completed = 0;
>>>> +       struct ib_wc *wcs;
>>>> +
>>>> +       if (poll_wc) {
>>>> +               wcs = poll_wc;
>>>> +               ib_poll_batch = batch;
>>>> +       } else {
>>>> +               wcs = cq->wc;
>>>> +               ib_poll_batch = IB_POLL_BATCH;
>>>> +       }
>>>
>>> Since this code has to be touched I think that we can use this
>>> opportunity to get rid of the "poll_wc ? : cq->wc" conditional and
>>> instead use what the caller passes. That will require to update all
>>> __ib_process_cq(..., ..., NULL) calls. I also propose to let the caller
>>> pass ib_poll_batch instead of figuring it out in this function.
>>> Otherwise the approach of this patch looks fine to me.
>>
>> Thanks Bart.
>> I'll make these changes and submit.
> 
> That sounds reasonable to me too, thanks for reworking and resubmitting.
> 

Sure, NP.
We've run NVMe-oF and SRP with the new patch.
I'll send it through Mellanox maintainers pull request.

Thanks for reporting and reviewing.

-Max.