On 03/22/2018 10:37 AM, Pierre Morel wrote: > 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! np >> >>> 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. Works with me. > > 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. > " Sounds good to me. Halil
