Re: [PATCH 1/4] vfio: ccw: fix cleanup if cp_prefetch fails

[Pierre Morel]

On 22/03/2018 03:22, Dong Jia Shi wrote:
> * Halil Pasic <pasic@xxxxxxxxxxxxxxxxxx> [2018-03-21 13:49:54 +0100]:
>
> > On 03/21/2018 03:08 AM, Dong Jia Shi wrote:
> > > From: Halil Pasic <pasic@xxxxxxxxxxxxxxxxxx>
> > >
> > > If the translation of a channel program fails, we may end up attempting
> > > to clean up (free, unpin) stuff that never got translated (and allocated,
> > > pinned) in the first place.
> > >
> > > By adjusting the lengths of the chains accordingly (so the element that
> > > failed, and all subsequent elements are excluded) cleanup activities
> > > based on false assumptions can be avoided.
> > >
> > > Let's make sure cp_free works properly after cp_prefetch returns with an
> > > error by setting ch_len to 0 for the ccw chains those are not prefetched.
> > This sentence used to be:
> >
> > Let's make sure cp_free works properly after cp_prefetch returns with an
> > error.
> >
> > @Dong Jia
> > I find the 'by setting ch_len to 0 for the ccw chains those are not prefetched'
> > you added for clarification (I guess) somewhat problematic.
> > The chain in which the translation failure occurred
> > +	chain->ch_len = idx;
> I made a mistake. When rewording the message, I missed this part...
> Sorry for the problem!
>
> > is shortened so that only the translated elements (ccws) are going to
> > get cleaned up (on a per element basis) by cp_free. This may or may
> > not be the first ccw. Subsequent chains are shortened to 0 as there
> > no translation took place.
> >
> > So as a result of this change only properly translated ccws are going
> > to get (re)visited by cp_free as only those may have resources bound
> > to them which need to be released.
> >
> > I'm not against improving the commit message. But this ain't
> > an improvement to me.
> You are right. How about:
> Let's make sure cp_free works properly after cp_prefetch returns with an
> error by setting ch_len of a ccw chain to the number of the translated
> ccws on that chain.

By the way, since you will propose a new version,
you have a long description of the cp_prefetch function in the code.
I think you should modify it according to the changes and describe how and
why the ch_len field of each chain is used and changed by this function.

Something like:

"
For each chain composing the channel program:
On entry ch_len hold the count of CCW to be translated.
On exit ch_len is adjusted to the count of successfully translated CCW.

This allows cp_free to find in ch_len the count of CCW to free in a chain.
"

Could also be inside the commit message.

Pierre


> > > Acked-by: Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx>
> > > Reviewed-by: Dong Jia Shi <bjsdjshi@xxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Halil Pasic <pasic@xxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Dong Jia Shi <bjsdjshi@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/s390/cio/vfio_ccw_cp.c | 9 ++++++++-
> > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> > > index d9a2fffd034b..2be114db02f9 100644
> > > --- a/drivers/s390/cio/vfio_ccw_cp.c
> > > +++ b/drivers/s390/cio/vfio_ccw_cp.c
> > > @@ -749,11 +749,18 @@ int cp_prefetch(struct channel_program *cp)
> > >   		for (idx = 0; idx < len; idx++) {
> > >   			ret = ccwchain_fetch_one(chain, idx, cp);
> > >   			if (ret)
> > > -				return ret;
> > > +				goto out_err;
> > >   		}
> > >   	}
> > >
> > >   	return 0;
> > > +out_err:
> > > +	/* Only cleanup the chain elements that where actually translated. */
> > > +	chain->ch_len = idx;
> > > +	list_for_each_entry_continue(chain, &cp->ccwchain_list, next) {
> > > +		chain->ch_len = 0;
> > > +	}
> > > +	return ret;
> > >   }
> > >
> > >   /**

--
Pierre Morel
Linux/KVM/QEMU in BÃblingen - Germany