linux-kernel - Re: [PATCH v2] netlink: Replace rhash

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: <20150924024608.GA25502@htj.duckdns.org>
Date:	Wed, 23 Sep 2015 22:46:08 -0400
From:	Tejun Heo <tj@...nel.org>
To:	Herbert Xu <herbert@...dor.apana.org.au>
Cc:	David Miller <davem@...emloft.net>, cwang@...pensource.com,
	tom@...bertland.com, kafai@...com, kernel-team@...com,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	torvalds@...ux-foundation.org, jiri@...nulli.us,
	nicolas.dichtel@...nd.com, tgraf@...g.ch, sfeldma@...il.com
Subject: Re: [PATCH v2] netlink: Replace rhash_portid with bound

Hello, Herbert.

On Thu, Sep 24, 2015 at 10:30:11AM +0800, Herbert Xu wrote:
> Well if someone provided helpers which
> 
> 1) uses smp_wmb and smp_rmb instead of full barriers;

This part is fine.

> 2) provides raw variants for the cases that barriers aren't needed

Hmm... It looks like I'm failing at communicating.  Lemme try again.
There are two situations where we do this.

1. When there are different locking contexts.  In this case, the write
   path is.  It's already protected by the spinlock so the barrier
   isn't necessary.

2. When the path is hot enough for the cost of smp_rmb() to matter and
   the specifics of individual deref allows for micro optimization and
   justifies the added overhead in terms of increased fragility,
   complexity and need for documentation.

In both cases, we want to make reasonable trade-offs like any other
choices we make.  We don't go off and run to one extreme or the other
just because barriers are involved.  One good measure to use is
whether the extra documentation necessary is justifiable.  In this
case, on each unprotected derefs, we want to explain why the
unprotected deref is okay and justified.

> then I'm more than happy to use them.
> 
> Having reviewed the situation again I'm even more convincend
> now that smp_load_acquire/smp_store_release aren't the appropriate
> primitives for us.  They are meant for situations that are similar
> to spin lock/unlock where you need to prevent all reads/writes from
> floating above or below the load/store, respectively.
>
> For our situation we only need write or read ordering, so they are
> literally the wrong tool for the job and will only cause confusion
> in future when someone tries to do a major rewrite of the code and
> they will be scratching their head as to why we needed locking-like
> semantics here.

store_release/load_acquire vs. wmb/rmb is a separate issue.  I no
longer have objections against using wmb/rmb pairs here although I do
wanna note that eventually I think release/acquire are likely to be
more prevalent but that's a separate discussion.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/