netdev - Re: [PATCH net-next v6 04/12] net: homa: create homa_pool.h and homa

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXJAmy3QB84Jp7nTCF_4s0Cwj6Yg4csX+aHUz7qYpw+QsvOcw@mail.gmail.com>
Date: Sat, 25 Jan 2025 21:33:14 -0800
From: John Ousterhout <ouster@...stanford.edu>
To: Andrew Lunn <andrew@...n.ch>
Cc: Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, edumazet@...gle.com, 
	horms@...nel.org, kuba@...nel.org
Subject: Re: [PATCH net-next v6 04/12] net: homa: create homa_pool.h and homa_pool.c

On Fri, Jan 24, 2025 at 4:46 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> > > > +     homa_sock_lock(pool->hsk, "homa_pool_allocate");
> > >
> > > There is some chicken-egg issue, with homa_sock_lock() being defined
> > > only later in the series, but it looks like the string argument is never
> > > used.
> >
> > Right: in normal usage this argument is ignored. It exists because
> > there are occasionally deadlocks involving socket locks; when that
> > happens I temporarily add code to homa_sock_lock that uses this
> > argument to help track them down. I'd prefer to keep it, even though
> > it isn't normally used, because otherwise when a new deadlock arises
> > I'd have to modify every call to homa_sock_lock in order to add the
> > information back in again. I added a few more words to the comment for
> > homa_sock_lock to make this more clear.
>
> CONFIG_PROVE_LOCKING is pretty good at finding deadlocks, before they
> happen. With practice you can turn the stack traces back to lines of
> code, to know where each lock was taken. This is why no other part of
> Linux has this sort of annotate with a string indicating where a lock
> was taken.
>
> You really should have CONFIG_PROVE_LOCKING enabled when doing
> development and functional testing. Then turn it off for performance
> testing.

This makes sense. I wasn't aware of CONFIG_LOCKDEP or
CONFIG_PROVE_LOCKING until Eric Dumazet mentioned them in a comment on
an earlier version of this patch. I've had them set in my development
environment ever since, and I agree that the extra annotations
shouldn't be necessary anymore. I'll take them out.

-John-