Re: [PATCH rdma-next 01/10] RDMA: Restore ability to fail on PD deallocate

[Leon Romanovsky <leon@kernel.org>]

On Wed, Aug 26, 2020 at 12:49:03AM +0000, Saleem, Shiraz wrote:
> > Subject: Re: [PATCH rdma-next 01/10] RDMA: Restore ability to fail on PD
> > deallocate
> >
> > On 25/08/2020 16:44, Jason Gunthorpe wrote:
> > > On Tue, Aug 25, 2020 at 04:32:57PM +0300, Gal Pressman wrote:
> > >>> For uverbs it will go into an infinite loop in
> > >>> uverbs_destroy_ufile_hw() if destroy doesn't eventually succeed.
> > >>
> > >> The code breaks the loop in such cases, why infinite loop?
> > >
> > > Oh, that is a bug, it should WARN_ON when that happens, because the
> > > driver has triggered a permanent memory leak.
> >
> > Well, a WARN_ON won't do much good if you're stuck in an infinite loop :), the
> > break is definitely needed there.
> >
> > >>> For kernel it will trigger WARN_ON's and then a permanent memory leak.
> > >>>
> > >>>> I agree that drivers shouldn't fail destroy commands, but you
> > >>>> know.. bugs/errors happen (especially when dealing with hardware),
> > >>>> and we have a way to propagate them, why do it for only some of the
> > drivers?
> > >>>
> > >>> There is no way to propogate them.
> > >>>
> > >>> All destroy must eventually succeed.
> > >>
> > >> There is no way to propagate them on process cleanup, but the destroy
> > >> verbs have a return code all the way back to libibverbs, which we can
> > >> use for error propagation.
> > >
> > > It is sort of OK for a driver to fail during RDMA_REMOVE_DESTROY.
> > >
> > > All other reason codes must eventually succeed.
> > >
> > >> The cleanup flow can either ignore the return value, or we can add
> > >> another parameter that explicitly means the call shouldn't fail and
> > >> all allocated memory/state should be freed.
> > >
> > > I don't really see the value to return the error code to userspace, it
> > > would require churning all the drivers and all the destroy functions
> > > to pass the existing reason in.
> > >
> > > Since all the details of the FW failure reason are lost to some EINVAL
> > > (or already logged to dmesg) I don't see much point.
> >
> > Right, as always, the error code would probably not contain much information, but
> > there's a big difference between returning error code X/Y vs returning success
> > instead of an error. To me that just feels wrong, at least in cases where we can
> > prevent that.
> >
>
> The API is quite confusing now. If drivers are not expected to fail the destroy
> and there is no way to propagate the device failures, then the return type should be a void.
>
> Do we really want to have mixed/ambiguous definition of the API to support the quirks of one type of device?

This is in-kernel API and it can be imperfect, because we are not
bounded to not-break-userspace rule.

I don't like the current situation either, just don't know how to
solve it differently.

Thanks

>
> Shiraz
>