Re: [Discuss-gnuradio] Regarding lock protection when setting private variables in gnuradio blocks

[Marcus Müller]

-----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 Anastasopoulos 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
> 
> On Wed, Oct 15, 2014 at 11:59 AM, Tom Rondeau <address@hidden>
> wrote:
> 
>> On Wed, Oct 15, 2014 at 11:49 AM, Achilleas Anastasopoulos < 
>> address@hidden> wrote:
>> 
>>> 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
>>> 
>> 
>> 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 address@hidden 
> https://lists.gnu.org/mailman/listinfo/discuss-gnuradio
> 
-----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-----