> > Where do these not-freed skb come from? Except these skb, are other > resources freed correctly? > > IMHO, the root cause should be found and other resources should be also handled. > WARN_ON_ONCE() should be kept on exit and done. > > So if some skb are not freed incorrectly, we can find these skb in the loop end. > > Zhu Yanjun > In the original code (before the patch that this patch is trying to fix) there was one path through rxe_completer() that *could* have leaked an skb. We haven't been seeing that one though. Every other path through the state machine exited after calling kfree_skb(skb). You can check but you have to look at an older kernel. The earlier "Fix FIXME" patch, which is upstream, collected the code related to freeing the skbs into a subroutine called free_pkt(). This was an improvement but even though it calls kfree_skb(skb) it didn't set the local variable skb to NULL. This allowed some bogus warnings to show up as you have been demonstrating. This patch "fix reference counting (again)" fixes both of these issues by getting rid of the warning and moving the free_pkt() to the end of rxe_completer(). Now, after applying the patch, the end of the subroutine reads ... done: if (pkt) free_pkt(pkt); rxe_drop_ref(qp); return ret; } If you check, you can see there are no other return statements in the routine. (skb and pkt are pointers to the same packet. the macro SKB_TO_PKT sets pkt = &skb->cb and PKT_TO_SKB sets skb = container_of(...) so passing pkt is equivalent to passing skb.) The only way out, therefore, frees a packet if there is one and we cannot be leaking skbs. The free_pkt() routine handles all the other stuff that needs to be done when the skb is freed. (The rxe_drop_ref(qp) drops the reference to QP taken at the top of the rxe_completer() routine.) The default behavior when receiving an ack packet that isn't expected or perfectly correct is to drop it. This includes ack packets received when the sender is retrying a request. The comment after case COMPST_ERROR_RETRY: is misleading because you do have an ack packet when you get there but it could be unexpected if no wqe is waiting for it. This was the path that before could have lead to leaking an skb but that is now fixed. As of now there are no remaining paths that can leak a packet and no reason to check that skb == NULL. If it makes you happy you can clear it like this if (pkt) free_pkt(pkt); skb = NULL; WARN_ON_ONCE(skb); but that would be a pointless waste of time. Bob
