On 11/07/19 1:10 AM, Jon Maloy wrote: >> -----Original Message----- >> From: Eric Dumazet <eric.duma...@gmail.com> >> Sent: 10-Jul-19 04:00 >> To: Jon Maloy <jon.ma...@ericsson.com>; Eric Dumazet >> <eric.duma...@gmail.com>; Chris Packham >> <chris.pack...@alliedtelesis.co.nz>; ying....@windriver.com; >> da...@davemloft.net >> Cc: net...@vger.kernel.org; tipc-discuss...@lists.sourceforge.net; linux- >> ker...@vger.kernel.org >> Subject: Re: [PATCH] tipc: ensure skb->lock is initialised >> >> >> >> On 7/9/19 10:15 PM, Jon Maloy wrote: >>> >>> It is not only for lockdep purposes, -it is essential. But please provide >>> details >> about where you see that more fixes are needed. >>> >> >> Simple fact that you detect a problem only when skb_queue_purge() is called >> should talk by itself. >> >> As I stated, there are many places where the list is manipulated _without_ >> its >> spinlock being held. > > Yes, and that is the way it should be on the send path. > >> >> You want consistency, then >> >> - grab the spinlock all the time. >> - Or do not ever use it. > > That is exactly what we are doing. > - The send path doesn't need the spinlock, and never grabs it. > - The receive path does need it, and always grabs it. > > However, since we don't know from the beginning which path a created > message will follow, we initialize the queue spinlock "just in case" > when it is created, even though it may never be used later. > You can see this as a violation of the principle you are stating > above, but it is a prize that is worth paying, given savings in code > volume, complexity and performance. > >> >> Do not initialize the spinlock just in case a path will use skb_queue_purge() >> (instead of using __skb_queue_purge()) > > I am ok with that. I think we can agree that Chris goes for that > solution, so we can get this bug fixed.
So would you like a v2 with an improved commit message? I note that I said skb->lock instead of head->lock in the subject line so at least that should be corrected.
