> 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? Shiraz
