Are strategies not effective?

[akerbos]

I do not quite see through the code in this regard, so I might be wrong: are the strategies obeyed with respect to all already reserved cpus or only to the one last reserved? The latter is definitely not sufficient, therefor I ask.

[peter-lawrey]

In acquireLock the critical loop is

for (int i = PROCESSORS - 1; i > 0; i--) {
    AffinityLock al = LOCKS[i];
    if (al.canReserve() && strategy.matches(cpuId, i)) {
        al.assignCurrentThread(bind, false);
        return al;
    }
}

Only threads which can be reserved (are available and not used) are checked.

[akerbos]

I think you misunderstood my question.

Let us assume we apply the strategy "DIFFERENT_CORE" and four cores with 2 CPUs each (assuming natural numbering). Now we have three threads requesting a binding. Of course, the first is assigned CPU 8 on core 4 and the second CPU 6 on core 3. Now, will the third be assigned CPU 4 on core 4, which is the first CPU to be checked that fulfills the strategy w.r.t. both already assigned threads? Or will it be assigned to CPU 7 on core 4, which is the first CPU to be checked which fulfills the strategy w.r.t. to the lock given out last?

In the code this means: for which values of cpuId do you check?

[peter-lawrey]

It will be a DIFFERENT_CORE to the one you ask for a different core from. It allocates in descending order so it will use all the different cores, until it has to reuse the same cores again. This is entirely dependant on how the cpus are laid out.

[akerbos]

Ok, I see; obviously the lock I call aquireLock on checks (only) against itself. However, I think this is not good enough. In above scenario, AffinityThreadFactory will pick the wrong one, doesn't it? It checks against the lock last obtained so only that one will be checked. I think it should remember all locks issued (and still bound) and check against all of them.

[peter-lawrey]

If you need to have a dedicated core, you can use acquireCore() This will reserve a whole core to your thread.

[akerbos]

Yes, but AffinityThreadFactory does not. I would expect it to assign four different cores to four created threads if I pass DIFFERENT_CORE as first strategy, but it won't.

[peter-lawrey]

If there are free cores, it will chose a different one each time even if you just use ANY. While it does guarantee a core which different the all the locks so far i.e. it will not fail if there is a duplicate with a previously assigned core. The difference is rather subtle. e.g. it considers core 3 thread 1, core 2 thread 1, core 1 thread 1, core 0 thread 1 and only then will consider thread 0 on each core. It is only when there is no cores free does it consider using a thread on a core which is used. It appears you would prefer it to fail at this point. Instead DIFFERENT_CORE will just give you a thread on a core different to the one you have chosen.

[akerbos]

If I follow the method chain from AffinityThreadFactory#newThread and assume a strategy accepts a logical CPU, I end up at a call assignCurrentThread(false, false) on the corresponding lock. In particular, the core is not bound/locked. Therefore, my understand is that a subsequent call to newThread might result in locking on a CPU that shares core with a lock formerly issued (see above scenario).

Am I misreading something?

[peter-lawrey]

Let me check.

[peter-lawrey]

You ask lots of interesting questions.

/**
 * Assigning the current thread has a side effect of preventing the lock being used again until it is released.
 *
 * @param bind whether to bind the thread as well
 * @param wholeCore whether to reserve all the thread in the same core.
 */
private void assignCurrentThread(boolean bind, boolean wholeCore) {

[peter-lawrey]

I have added the test to AffinityLockTest

    @Test
    public void testIssue21() {
        AffinityLock al = AffinityLock.acquireLock();
        AffinityLock alForAnotherThread = al.acquireLock(AffinityStrategies.ANY);
        AffinityLock alForAnotherThread2 = al.acquireLock(AffinityStrategies.ANY);
        assertNotSame(alForAnotherThread, alForAnotherThread2);
        assertNotSame(alForAnotherThread.cpuId(), alForAnotherThread2.cpuId());

        al.release();
        alForAnotherThread.release();
        alForAnotherThread2.release();
    }

[akerbos]

This does not test the behaviour I am concerned about (you do not use DIFFERENT_CORE and you do only compare logical CPU identifiers, not core/socket ids). Consider this:

@Test 
public void testIssue21() {
  final CpuLayout layout = AffinityLock.cpuLayout();
  final AffinityLock al1 = AffinityLock.aquireLock();
  final AffinityLock al2 = al1.aquireLock(AffinityStrategies.DIFFERENT_CORE);
  final AffinityLock al3 = al2.aquireLock(AffinityStrategies.DIFFERENT_CORE);

  assertNotSame(layout.coreId(al1.cpuId()), layout.coreId(al2.cpuId()));
  assertNotSame(layout.coreId(al2.cpuId()), layout.coreId(al3.cpuId()));
  assertNotSame(layout.coreId(al1.cpuId()), layout.coreId(al3.cpuId())); // This is the important one!
}

(The test will always fail if you have less than three cores, of course.)

This is what AffinityThreadFactory does when creating it with DIFFERENT_CORE and calling newThread three times. As there is no way to check what locks a thread holds when you only have factory and thread objects, I can not write a unit test for the particular scenario. Above test does not have to go green, but it should serve the point.

However, AffinityThreadFactory#newThread should, imho, only aquire locks that conform to the given strategies and all previously aquired (and still active) locks.

[peter-lawrey]

As I said, this happens to work due to the way it searches for free threads, though you can't enforce it. This is because it always considers a new core each time any way. i.e. the following will work if you have three free cores.

  final CpuLayout layout = AffinityLock.cpuLayout();
  final AffinityLock al1 = AffinityLock.aquireLock();
  final AffinityLock al2 = al1.aquireLock(AffinityStrategies.ANY);
  final AffinityLock al3 = al2.aquireLock(AffinityStrategies.ANY);

  assertNotSame(layout.coreId(al1.cpuId()), layout.coreId(al2.cpuId()));
  assertNotSame(layout.coreId(al2.cpuId()), layout.coreId(al3.cpuId()));
  assertNotSame(layout.coreId(al1.cpuId()), layout.coreId(al3.cpuId())); // This is the important one!

[akerbos]

I do not see this in the code, but I can take your word for it.

[peter-lawrey]

Its not guaranteed in code as you say and some machine/OSes (I don't have any examples) could number their cpus in a different order and this wouldn't hold. In all the hyperthreaded examples I have, you have

cpu0 - cpuN - the thread 0 for each core cpuN - cpuM - the thread 1 for each core.

In reverse order (or forward if it used that) it will consider each free core before considering a thread of a previous used thread.

[akerbos]

Ahhhh, I see. I assumed that they would be ordered 0.0, 0.1, 1.0, 1.1, ...

Is it hard to ensure correct behaviour in the code?

[peter-lawrey]

It gets complicated if you are using a mix of behaviour e.g. DIFFERENT_SOCKET, SAME_CORE, SAME_SOCKET and for no known system would it make any difference.

[akerbos]

In this case, you should at least document the assumptions prominently so everybody knows what they are in for.

[peter-lawrey]

That assumes that I know all the assumptions I have made ;) and know what people want to know.

Since you have raised it as a concern I will add it to the How it Works page.