On Fri, Dec 20, 2019 at 06:12:51PM +0800, Liuyixian (Eason) wrote: > > > On 2019/12/18 22:00, Jason Gunthorpe wrote: > > On Mon, Dec 16, 2019 at 08:51:03PM +0800, Liuyixian (Eason) wrote: > >> Hi Jason, > >> > >> I want to make sure that is there any further comments on this patch set? > > > > I still dislike it alot. > > > > Hi Jason, > > Thanks for reply :) > Let us recall the previous discussions and check is there anything is pending on. > > Comments in patch set V1: > 1: why do you need a dedicated HIGHPRI work queue? > 2. As far as I could tell the only thing the triggered the work to run > was some variable which was only set in another work queue 'hns_roce_irq_work_handle()' > 3. don't do allocations at all if you can't allow them to fail. > > Comments in patch set V2: > 1. It kind of looks like this can be called multiple times? It won't work > right unless it is called exactly once. > 2. Why do you need more than one work in parallel for this? Once you > start to move the HW to error that only has to happen once, surely? > Flush operation should be implemented once the QP state is going to > be error or the producer index is updated for the QP in error state. > 3. The work function does something that looks like it only has to happen > once per QP. One do you need to keep re-queing this thing every time the user posts > a WR? > wqe every time the PI is updated. That's re-queuing is needed. > > In conclusion, we have accepted all the comments in V1, and explained to all 3 comments in V2 > but no further response. Do I have missed to cover any of your concerns? > > I would really appreciate it if you could help to point out the unsolved issue. You explained you need to push the latest PI to the HW, so I still don't understand why it needs to be coded in such a confusing Keep a note if the PI is being pushed, if not, start pushing it. If yes, update that PI that will be pushed and do nothing Jason
