Re: [PATCH] IB/core: Make ib_dealloc_pd return void

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

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

On 08/05/2015 04:34 PM, Jason Gunthorpe wrote:
> The majority of callers never check the return value, and even if they
> did, they can't do anything about a failure.
> 
> All possible failure cases represent a bug in the caller, so just
> WARN_ON inside the function instead.
> 
> This fixes a few random errors:
>  net/rd/iw.c infinite loops while it fails. (racing with EBUSY?)
> 
> This also lays the ground work to get rid of error return from the
> drivers. Most drivers do not error, the few that do are broken since
> it cannot be handled.
> 
> Since uverbs can legitimately make use of EBUSY, open code the
> check.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> ---
>  drivers/infiniband/core/uverbs_cmd.c       | 22 ++++++++++++++++------
>  drivers/infiniband/core/verbs.c            | 26 ++++++++++++++++++++------
>  drivers/infiniband/ulp/ipoib/ipoib_verbs.c |  4 +---
>  drivers/infiniband/ulp/iser/iser_verbs.c   |  2 +-
>  include/rdma/ib_verbs.h                    |  6 +-----
>  net/rds/iw.c                               |  5 +----
>  net/sunrpc/xprtrdma/verbs.c                |  2 +-
>  7 files changed, 41 insertions(+), 26 deletions(-)
> 
> Doug, this applies after the local_dma_lkey cleanup series.
> 
> Lightly tested with ipoib/uverbs/umad on mlx4.
> 
> This patch will expose buggy ULPs, I would not be too surprised to see
> a report of the WARN_ON triggering...

I'm not sure I like this patch.  I have my concerns that this will make
debugging and catching problems more difficult.  In particular, in the
recent past when I was working on the IPoIB driver and its
startup/shutdown sequence, I wrote this changelog:

This factoring is needed in preparation for having per-device work
queues.  The original per-device workqueue code resulted in the
following error message:

    <ibdev>: ib_dealloc_pd failed

The change you put in here will make note of the driver that fails to
return a clean return, but the error happens elsewhere.  This obscures
the source of the error.  And it's not out of the question (or even
unlikely) that shifts in how people do transfer engines might result in
lost allocations and this warning popping up.  It serves as a final
check on the ULP, and to me that has value.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]