Multithreading - synchronize or not?

[mika]

I created this example based of my multithreading app.

How to do synchronization in this? I kinda works with no syncronization at all...

relevant snipet from attached example:

Code: Pascal  [Select][+][-]

1. procedure TThreadEven.Execute;
2. var cur : qword;
3.     k: longword;
4.     OddVal : qword;
5.     EvenVal : qword;
6.     er : longword;
7. begin
8.      OddVal:= 1;
9.      EvenVal:=1;
10.      while (not Terminated)  do
11.      begin
12.           inc(Counter); //-- spin count
13.  
14.           if FlagEven = 0 then
15.           begin
16.                FlagEven:=1;
17.                for k:=0 to cBigSize-1 do aEven[k]:=EvenVal;
18.                inc(EvenVal);
19.                inc(CountOutData);
20.                FlagEven:=2; //-- send data
21.           end;
22.  
23.           if FlagOdd = 2 then //-- incoming data from "Odd" thread
24.           begin
25.                er:=0;
26.                FlagOdd:=3;
27.                for k:=0 to cSize-1 do if aOdd[k]<>OddVal then inc(er);
28.                inc(OddVal);
29.                inc(CountInData);
30.                if er<>0 then inc(CountErData);
31.                FlagOdd:=0; //-- data recived and processed, ready for new
32.           end;
33.      end;
34. end;
35.  
36.  
37. procedure TThreadOdd.Execute;
38. var cur : qword;
39.     k: longword;
40.     OddVal : qword;
41.     EvenVal : qword;
42.     er : longword;
43.  
44. begin
45.      OddVal:= 1;
46.      EvenVal:=1;
47.      while (not Terminated)  do
48.      begin
49.           inc(Counter); //-- spin count
50.  
51.           if FlagOdd = 0 then
52.           begin
53.                FlagOdd:=1;
54.                for k:=0 to cSize-1 do aOdd[k]:=OddVal;
55.                inc(OddVal);
56.                inc(CountOutData);
57.                FlagOdd:=2; //-- send data
58.           end;
59.  
60.           if FlagEven = 2 then //-- incoming data from "Even" thread
61.           begin
62.                er:=0;
63.                FlagEven:=3;
64.                for k:=0 to cBigSize-1 do if aEven[k]<>EvenVal then inc(er);
65.                inc(EvenVal);
66.                inc(CountInData);
67.                if er<>0 then inc(CountErData);
68.                FlagEven:=0; //-- data recived and processed, ready for new
69.           end;
70.  
71.      end;
72. end;

[Martin_fr]

Well, only looking at the extract in the post...

The global counters have a risk of going wrong. (race condition).

You should at least use

Code: Pascal  [Select][+][-]

1. InterlockedIncrement(Counter)

And similar for other counters.


I don't know if "aEven" is shared with something?

[Martin_fr]

Code: Pascal  [Select][+][-]

1. FlagEven

That should work...

Well, depends: Is there exactly ONE even thread, and exactly ONE odd thread? => Or are there several of at least one of them.

If there are several, then it is a problem.
If there are several then you need (iirc)

Code: Pascal  [Select][+][-]

1. InterlockedCompareExchange(FlagEven, 1, 0)

Set it to 1, but only if it really still is 0. (Because, otherwise 2 threads can set it to 1.)
And use the result of that for the "if" condition

If there is ONLY ONE thread of each then... Well, afaik technically you need Read/Write barriers in some cases. But it may work without. Though, I would put a "WriteBarrier;" in front of the assignments setting it to 2 or 0 (before the "end" of the "if"'s begin/end block). 

Interlocked, does include the needed barriers.

--- EDIT:

Well actually, if you have ONLY ONE of each thread. You may need ReadWriteBarrier after entering each if block.

[mika]

"Counter" is member of thread class. It is local to thread, there is no conflict.

aEven, aOdd is global array, that one thread writes and other reads. Flag indicates when to read when to write.

If there is ONLY ONE thread of each then... Well, afaik technically you need Read/Write barriers in some cases. But it may work without. Though, I would put a "WriteBarrier;" in front of the assignments setting it to 2 or 0 (before the "end" of the "if"'s begin/end block). 
yes it is  "ONLY ONE thread of each" and ONLY ONE at a time.

Interlocked, does include the needed barriers.

"Interlocked" you mean:
lock cmpxchg [mem], reg
or
lock xadd [mem], reg ?

[Martin_fr]

"Counter" is member of thread class. It is local to thread, there is no conflict.

aEven, aOdd is global array, that one thread writes and other reads. Flag indicates when to read when to write.

So 2 threads total only?

Then why set               

Code: Pascal  [Select][+][-]

1. FlagEven:=1;

There is no thread checking for that. The thread that wanted it 0 is in the "if" block, the other one will wait till it is 2. So you could leave it 0?

Interlocked, does include the needed barriers.

"Interlocked" you mean:
lock cmpxchg [mem], reg
or
lock xadd [mem], reg ?

Both of them include (afaik) all the read/write barriers that you need. Any InterLocked... (or asm lock ...) should do that.

[Martin_fr]

https://en.wikipedia.org/wiki/Memory_barrier

So between modifying the data in "aEven" and setting "EvenFlag" for the other thread you need a barrier. (write barrier).

After checking the flag, and before reading the data, you need a readbarrier.

Assuming ONLY ONE thread each:

Code: Pascal  [Select][+][-]

1.           if FlagEven = 0 then  // this can be a dirty read, no problem
2.           begin
3. //               FlagEven:=1; // not needed
4. ReadWriteBarrier; // Don't read the below data before FlagEven really is 0
5.        // Don't write it either....
6.                for k:=0 to cBigSize-1 do aEven[k]:=EvenVal;
7. // If I am correct then the other thread won't access them due to FlagEven => so "inc" is ok, no InterlockedIncrement needed)
8.                inc(EvenVal);
9.                inc(CountOutData);
10. WriteBarrier; // Don't write the below, before all the above has been written
11.                FlagEven:=2; //-- send data
12.           end;

If you only write to aEven, so you may only need a WriteBarrier on entry into the "if" block. You can check for each block if you read, write, or read/write shared data ... Though the "inc" is a read/write...



You still need InterlockedIncrement for the Counter that is not protected by any flags.

[mika]

https://en.wikipedia.org/wiki/Memory_barrier

Assuming ONLY ONE thread each:

...
If you only write to aEven, so you may only need a WriteBarrier on entry into the "if" block. You can check for each block if you read, write, or read/write shared data ... Though the "inc" is a read/write...

yes "ONLY ONE thread each", ONLY ONE  at a time

FlagEven:=1; is for perfomance measurments with "EverSpin" thread thats count time spent no each flag state. Agree, not needed in final product.

I did try memory barrier like "sfence" , "lfence", "mfence".. but i can not mesure diference in perfomance. Results are inconsitent as is, and effect of "fneces" is not distinguishable.  maybe i have wrongly constructed exaple.

From documentation about "fence" i understand that they are local to thread instructions and only help to keep in check "out of order" execution of instructions. Nothing about faster or forced data synchroniztion with main memory.

[Thaddy]

barriers are supported in system:
https://www.freepascal.org/docs-html/rtl/system/readwritebarrier.html