Regarding lock protection when setting private variables in gnuradio blocks

GNU Radio
1

My question arose from a comment that Jonathan made on one of the pull
requests
in gnuradio (#293).

If we have a set function in a gr block that sets some private variable
that is
used in the work function, do we need to protect it to make the whole
operation thread safe?

Is this a standard practice in gnuradio blocks?
eg, why is this not used in “add_const_vXX_impl.h.t” ?

thanks
Achilleas

2

On Wed, Oct 15, 2014 at 11:49 AM, Achilleas A. <
[email protected]> wrote:

eg, why is this not used in “add_const_vXX_impl.h.t” ?

thanks
Achilleas

If it’s not atomic, then yes, you really should protect it. All blocks
have
a mutex called d_setlock you can easily use for this:

  gr::thread::scoped_lock lock(d_setlock);

Tom

3

Is “forecast()”" need to be protected in such a case as well?

just searching on the web i realized that no operation can be assumed
atomic, so
I guess every single set_ method (even if it assigns a
float/int/short/char) needs to be protected…is this an overkill?

Achilleas

4

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Achilleas,

short answer: no, it doesn’t need protection.
long answer: as long as the only one calling forecast() is the
thread-per-block (==default) scheduler, don’t worry, it will never
concurrently run with work(), because it only gets called when the
block is “idling”.

Generally, it seems that you mentally invert what you need to protect
against concurrent access:
You protect class members, not methods. A method might contain an
assignment that you’d want to protect; in that case, use a scoped lock
(like Tom illustrated).

Generally, consider it to be good style to protect every variable that
will break your running work function when it changes mid-operation.
For example, if you want to use a class member like d_length_preamble
in your work function in a loop condition:

… work(…) {
…
float samples[d_length_preamble];
for(float *sample = samples; sample != samples + d_length_preamble;
sample++) {
*sample = do_some_calculation();
}
…
}

void set_preamble_length(unsigned int length){
d_length_preamble = length;
}

then ostentatiously, changing d_length_preamble while work is running
might be totally disastrous. So an assignment being atomic is not the
problem here, the question is if /access/ to the variable should be
considered atomic. In the example above, it’s not: the loop depends on
the value of d_length_preamble to not change midways through operation.

The solution to the problem above would of course be not doing
something stupid like checking a pointer against a single address, if
you could also just use “<” instead, or forfeit the pointer magic and
do everything with indices. But the point of the example was to show
you that it’s not the setter method that needs protection – it’s the
points where you access the member that need consideration.

So if you wanted to protect these accesses, you would insert a
gr::thread::scoped_lock lock(d_setlock);
before the “float samples[d_length_preamble];” line, as well as before
“d_length_preamble = length;”. As long as one of the locks is in
scope, the other one’s creation blocks. This way, accesses can’t
happen concurrently.

Generally, I think, since multithreading is a complex and yet
important issue, it might be best to grab a cup of tea and your
favourite book on object oriented software development – anything
released after 2000 should have a chapter on multithreading,
explaining semaphores/mutexes, conditions etc.

Greetings,
Marcus

On 15.10.2014 20:55, Achilleas A. wrote:

wrote:

Discuss-gnuradio Info Page

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUPt9nAAoJEAFxB7BbsDrLSFAH/10SeeH3ZBy68MX/o0MsdAkC
cLIF8xKVFQ9F8lNcs8BaiS/mPdDOxnJd4eHn0fmQuWpDYaoCZuOU8COzq9AZVWnz
EhIuL7M9/LJwMX5ietxb4WolwOZbtJInGupaLuSHak9GBHmmlTK8qfMR9NTMuPGX
blWtXEJ4xc2pkrFoo7nZS3GkohIj/gf9AbY96MI2YXP7knE9rHy+zUm+zl+JhccK
z6OG0psuPdGplI+POKzolV2lytI8gx3cldl4dA+wzEuvc4zPTvoCxnlhP7dilY73
78aGh1Jv9n5hoajWin3AGdaXw1tSCLdIXamYMQgBg4iBYRRHraN73vbzzdDC7n8=
=nO4u
-----END PGP SIGNATURE-----

5

On Wed, Oct 15, 2014 at 2:55 PM, Achilleas A.
<[email protected]

wrote:

Is “forecast()”" need to be protected in such a case as well?

just searching on the web i realized that no operation can be assumed
atomic, so
I guess every single set_ method (even if it assigns a
float/int/short/char) needs to be protected…is this an overkill?

Achilleas

Achilleas,

So no about the forecast issue. Unless your forecast method uses
non-atomic
variables that can be set using a function setter. Marcus’ response more
completely outlines why forecast is thread safe between itself and the
work
function.

On the atomic issue, yes, you’re technically correct that there really
is
no such thing as an atomic data type. C++11 more officially defines
concepts of atomic data types, and Boost introduced it’s atomic type (I
believe) in 1.53
(Chapter 4. Boost.Atomic - 1.53.0).
However, for all intents and purposes,
things like int, char, float, double, etc behave fine in multithreaded
environments. I’m trying to remember where I went over this in some
depth
at one point, but the result is that while they are not technically
atomic,
they behave correctly – and I wish I could explain more thoroughly why
right now.

We will be moving our Boost version requirements to >= 1.53 in our 3.8
release, which means we can start using their atomic type concept then.

Tom

6

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Marcus,

On 16.10.2014 16:54, [email protected] wrote:

Is it not the case that a given instance of a block is only ever
executed by a single thread, so instance variables

are completely safe to modify in-flight?

No; assume the following scenario:
block (instance) “estimator”, in its work function, thinks it’s cool
to update block (instance) “equalizer”'s internal state by calling
equalizer_sptr->set_taps(…). In the meantime, the equalizer’s work
is running.

so, now equalizer_sptr->set_taps runs in estimator’s thread, while it
modifies stuff that gets accessed from equalizer’s thread.

Now, the solution to that is either using mutexes (which will, if not
done on a very high granularity, lead to blocks running effectively
single-threaded, because blocks block each other)
OR employing some notifier/listener scheme and let the blocks’
executors work through the updates after their work run in their own
thread. That way, you end up with a variant of the “new” style async
message passing framework, where messages cannot interfere with
running work functions, and message handling is guaranteed to be
reentrant-safe.

Greetings,
Marcus

Is “forecast()”" need to be protected in such a case as well?
So no about the forecast issue. Unless your forecast method uses
float, double, etc behave fine in multithreaded environments. I’m

variable that is used in the work function, do we need to protect

Links: ------ [1]
Chapter 4. Boost.Atomic - 1.53.0 [2]
Discuss-gnuradio Info Page

_______________________________________________ Discuss-gnuradio
mailing list [email protected]
Discuss-gnuradio Info Page

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUP+TrAAoJEAFxB7BbsDrLqkUH/juNua+b3rID0Nt9/U75xX4K
uMsQNFK39OO3s3eACM/zFQQ6pL1g8sOVaHh76lwkfYP+bXS2sF+hnDmfLN9Zw5kG
VCbDWYuohMWBN0Ti0yMimocEDD9AO1IkaofC/p4/4HemlS6dNiJuviRWhbIki/6A
V9daWfs92nybtf610wy2k+QUqT+IpmKR+YdacE0wwHsmiBMwDU8+4lk76H/Wrclt
SG15zuvRHOcaBVfwY+Bkdnqub4e9uqc9JM8+Qo1+ayutkzQb5SeEWou1MbJ2QGdK
6SpsRGUSaffb/bMk06RZRJwazWZmboTRnIKC++FsIHmpvr72d/DGetOvlDfMe1I=
=fvJN
-----END PGP SIGNATURE-----

7

Discuss-gnuradio mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/discuss-gnuradio

8

Is it not the case that a given instance of a block is only ever
executed by a single thread, so instance variables

are completely safe to modify in-flight?

There are exceptions, of course, like in FFTW, only a single thread can
be “planning” at a time, due to the

way FFTW does its thing.

On 2014-10-16 09:58, Tom R. wrote:

If it’s not atomic, then yes, you really should protect it. All blocks have a
mutex called d_setlock you can easily use for this:

gr::thread::scoped_lock lock(d_setlock);

Tom

Discuss-gnuradio mailing list
[email protected]
Discuss-gnuradio Info Page [2]

Links:

9

Yeah, so, a little cerebral flatulence on my part.

I think the problems come in with variables that can “change shape”
during a setting operation, which could lead

to horrific badness if the “shape change” happens while the work
function is using the variable. Imagine a

private buffer whose size and, potentially, base-address, can change
during the work function.

The scalars are “safer”, but not completely “safe”, in that they could
have semantics with bad consequences if

they change during work function processing. Those bad consequences can
be either rather benign–for example

a gain changing during processing of samples–just leads to a glitch,
or much-worse, core-dumping territory.