From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liwei Song Date: Tue, 2 May 2017 09:00:41 +0800 Subject: [Intel-wired-lan] [PATCH] ixgbe: initialize u64_stats_sync structures early at ixgbe_probe In-Reply-To: References: <1480909244-16460-1-git-send-email-liwei.song@windriver.com> Message-ID: <5907DA39.4020006@windriver.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: Liwei On 04/26/2017 01:16 AM, Alexander Duyck wrote: > On Tue, Apr 25, 2017 at 8:39 AM, Lino Sanfilippo wrote: >> Hi, >> >>> This patch doesn't look right to me. I would suggest rejecting it. >>> >>> The call to initialize the stats should be done when the ring is >>> allocated, not in ixgbe_probe(). This should probably be done in >>> ixgbe_alloc_q_vector() instead. >>> >> AFAICS ixgbe_alloc_q_vector() is also called in probe() (by ixgbe_init_interrupt_scheme()). >> Furthermore it is also called in resume() which would lead to multiple initialization of >> the u64_stats_sync in case of resume. > ixgbe_alloc_q_vector() is what allocates the ring structures that are > being initialized here. Calling it anywhere other than here doesn't > make sense since what we want to do is initialize the memory after we > have allocated it, but before we hand the pointer to it over to a > netdev or in this case an adapter structure. > >> IMHO the u64_stats_sync variables have to be initialized before register_netdev() is called >> since this is the point from which userspace can call ixgbe_get_stats64(). I would say the >> best place to do so is the probe() function as it is done in this patch. > I would disagree here. We should be initializing the stats variables > after we allocate them. Especially since we can end up freeing and > reallocating them any time the number of queues is changed. > >> Just my 2 cents. >> >> Regards, >> Lino > My advice would be to look through the code and verify what it is you > need to initialize and where it should happen. In this case we are > getting a lockdep splat since we are just letting things get > initialized with kzalloc and aren't following up in the right place. I > don't disagree that the original code has the u64_stats_init in the > wrong place since we can open/close the interface and trigger a > reinitialization of the stats. I would say we need to initialize the > stats just after we allocate them in memory so that if we decide to > free and reallocate the rings we initialize the new rings before they > are added to the netdev and don't reintroduce this issue in just a > different form. Thanks for your suggestion, I will try to modify it according to this. Liwei > > - Alex > > -------------- next part -------------- An HTML attachment was scrubbed... URL: