On 2020/1/13 22:04, Jason Gunthorpe wrote:
> On Mon, Jan 13, 2020 at 07:26:45PM +0800, Liuyixian (Eason) wrote:
>>
>>
>> On 2020/1/10 23:26, Jason Gunthorpe wrote:
>>> On Sat, Dec 28, 2019 at 11:28:54AM +0800, Yixian Liu wrote:
>>>> +void init_flush_work(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp)
>>>> +{
>>>> + struct hns_roce_work *flush_work;
>>>> +
>>>> + flush_work = kzalloc(sizeof(struct hns_roce_work), GFP_ATOMIC);
>>>> + if (!flush_work)
>>>> + return;
>>>
>>> You changed it to only queue once, so why do we need the allocation
>>> now? That was the whole point..
>>
>> Hi Jason,
>>
>> The flush work is queued **not only once**. As the flag being_pushed is set to 0 during
>> the process of modifying qp like this:
>> hns_roce_v2_modify_qp {
>> ...
>> if (new_state == IB_QPS_ERR) {
>> spin_lock_irqsave(&hr_qp->sq.lock, sq_flag);
>> ...
>> hr_qp->state = IB_QPS_ERR;
>> hr_qp->being_push = 0;
>> ...
>> }
>> ...
>> }
>> which means the new updated PI value needs to be updated with initializing a new flush work.
>> Thus, maybe there are two flush work in the workqueue. Thus, we still need the allocation here.
>
> I don't see how you should get two? One should be pending until the
> modify is done with the new PI, then once the PI is updated the same
> one should be re-queued the next time the PI needs changing.
