Re: [PATCH rdma-next v1 05/10] RDMA: Restore ability to fail on SRQ destroy

[Leon Romanovsky <leon@kernel.org>]

On Wed, Sep 02, 2020 at 09:18:27PM -0300, Jason Gunthorpe wrote:
> On Sun, Aug 30, 2020 at 11:40:05AM +0300, Leon Romanovsky wrote:
>
> > -void mlx5_ib_destroy_srq(struct ib_srq *srq, struct ib_udata *udata)
> > +int mlx5_ib_destroy_srq(struct ib_srq *srq, struct ib_udata *udata)
> >  {
> >  	struct mlx5_ib_dev *dev = to_mdev(srq->device);
> >  	struct mlx5_ib_srq *msrq = to_msrq(srq);
> > +	int ret;
> > +
> > +	ret = mlx5_cmd_destroy_srq(dev, &msrq->msrq);
> > +	if (ret && udata)
> > +		return ret;
> >
> > -	mlx5_cmd_destroy_srq(dev, &msrq->msrq);
> > -
> > -	if (srq->uobject) {
> > -		mlx5_ib_db_unmap_user(
> > -			rdma_udata_to_drv_context(
> > -				udata,
> > -				struct mlx5_ib_ucontext,
> > -				ibucontext),
> > -			&msrq->db);
> > -		ib_umem_release(msrq->umem);
> > -	} else {
> > -		destroy_srq_kernel(dev, msrq);
> > +	if (udata) {
> > +		destroy_srq_user(srq->pd, msrq, udata);
> > +		return 0;
> >  	}
> > +
> > +	/* We are cleaning kernel resources anyway */
> > +	destroy_srq_kernel(dev, msrq);
>
> Oh, and this isn't right.. If we are going to leak things then we have
> to leak anything exposed for DMA as well, eg the fragbuf under the SRQ
> has to be leaked.

We are leaking for ULPs only, from their perspective everything was
released and WARN_ON() will be the sign of the problem.

>
> If the HW can't guarentee it stopped doing DMA then we can't return
> memory under potentially active DMA back to the system.

ULPs are supposed to guarantee that all operations stopped.

>
> IHMO mlx5, and all the other drivers, get this wrong. Failing to
> eventually destroy an object is a catastrophic failure of the
> device. In the case of a kernel object it must always be destroyed on
> the first attempt.

This is current flow, we are destroying kernel object anyway.

>
> In this case the device should be killed. Disable memory access at the
> PCI config space, trigger a device reset, disassociate the device, and
> allow all destroy commands to fake-succeed.
>
> Since drivers need help to get this right, I'm wonder if we should fix
> this at the core level by introducing a 'your device is screwed up,
> kill it' callback.
>
> Then all the destroys can return failures as Gal wanted.
>
> The core logic would be something like
>
> ret = dev->ops.destroy_foo()
> if (is_kernel_object())
>    dev->ops.device_is_broken()
>    ret = dev->ops.destroy_foo()
>    WARN_ON(ret);
>
> Ie after 'device_is_broken' the driver must always succeed future
> destroys.
>
> Then we have a chance to make this work properly... mlx5 at least
> already has an implementation of 'device_is_broken' that does trigger
> success for future destroys.

I don't know, all those years we relied on the ULPs to do destroy
properly and it worked well. I didn't hear any complain from the field
that HW destroy failed in proper ULP flow.

It looks to me over-engineering.

Thanks

>
> Jason