[squid-users] FATAL: assertion failed: mem/PageStack.cc:159: "StoredNode().is_lock_free()"

[Nishant Sharma]

On 03/07/24 21:27, Alex Rousskov wrote:
> On 2024-07-03 09:27, Nishant Sharma wrote:
>> I was able to compile by replacing `uint64_t` to `uint32_t` and squid 
>> worked with workers > 1.
> 
> Where did you replace uint64_t with uint32_t? In IdSet::Node typedef? 
> Any other changes? AFAICT, changing just IdSet::Node badly breaks the 
> corresponding binary tree code because we hard-code the number of bits 
> per leaf node (at least!) to be 64. I did not audit code for other 
> dependencies.

In the code for squid-6.10, in ipc/mem/PageStack.h there is just one 
occurrence of `uint64_t` at line 79 inside class IdSet. I had changed 
that. But now I understand that I don't have to, rolled it back.

class IdSet
{
public:
    using size_type = IdSetMeasurement::size_type;
    using Position = IdSetPosition;
...
...
...
private:
    typedef uint64_t Node; ///< either leaf or intermediate node
    typedef std::atomic<Node> StoredNode; ///< a Node stored in shared 
memory
...
...
    // No more data members should follow! See FlexibleArray<> for details.
};

>> Further discussion on Openwrt issue tracker suggested [1] the following:
>>
> 
> It is possible that the above comment was negatively influenced by the 
> previous misleading statement about Squid v4 having no uint64_t: "In 
> code for version 4.x, there is no mention of uint64_t. It was introduced 
> with 5.x." In reality, Squid has been using 64-bit integers since before 
> Squid v3. It is neither practical nor necessary to remove uint64_t from 
> Squid so there will be no corresponding conditionals in ./configure.

That was due to my comment there. I actually meant to convey that I 
couldn't find a reference to `uint64_t` in the PageStack.cc file as I 
simply knew that the assert error message is being generated from the 
code in this file.

> The shared binary tree (that contains the assertion) did not exist in 
> v4, as we discussed earlier.

Ack. This is the correct statement that I should have used while 
replying on Openwrt issue tracker.

>> Is there any change that we need to do in the configure script to 
>> check for the availability of 64 bit atomic lock and use 32 bit lock 
>> if not available?
> 
> It is technically possible (perhaps even without ./configure checks), 
> but it would require adjusting complex shared tree code in the abcense 
> of comprehensive ready-to-use tests. It is trivial to break that code. 
> It is difficult to detect bugs. IMO, we should not expose ourselves to 
> such risks in this case, especially since Squid uses 64-bit atomics in 
> many other places: Supporting 32 bits in shared binary tree nodes is not 
> going to remove the last frequently used 64-bit lock.

Just being curious here, if a certain platform (mips32 in this case) is 
unable to guarantee a 64 bit atomic lock, other functions except SMP 
mode might get affected as well?

>> Or may be document the fact that it is not advisable / possible to run 
>> squid in SMP mode on such platforms that are not able to provide 64 
>> bit lock ID.
> 
> I believe your experiments with removing the assertion point in a rather 
> different direction: If your tests do not suggest otherwise, we should 
> downgrade that assertion to a startup warning. Let folks run Squid on 
> platforms without 64-bit atomic locks (if they wish to do so), but warn 
> them about an uncertain impact. Perhaps we can even convince ourselves 
> that the impact can only be on performance (i.e., there can be no 
> deadlocks due to mutexes).
> 
> Disclaimer: I do not know what "lock ID" is in this context.

I am not a programmer and not very well versed with a lot of these 
terms, so I have mixed / messed up while passing messages between the 
two forums.

"lock ID" term was used on Openwrt issue tracker where it was suggested 
that "The assertion assumes 64bit lock id.". [1]

Let me experiment with squid-6.x on these devices and also use them in 
the live environment.

The only change being commenting out the following line from 
ipc/mem/PageStack.c:

`assertion(StoredNode().is_lock_free());`

I will report back with success or any failures encountered.

Regards,
Nishant

[1] 
"https://github.com/openwrt/packages/issues/24469#issuecomment-2202322703"