Trac November 2024

trac@lists.einsteintoolkit.org

1 participants
106 discussions

#2832: possible race condition in LoopControl
by Roland Haas 05 Nov '24

05 Nov '24

#2832: possible race condition in LoopControl Reporter: Roland Haas Status: new Milestone: Version: Type: bug Priority: major Component: Comment (by Roland Haas): @{557058:d079f9c2-ad27-47b2-bf4b-ecc6bbe288b0} could you give this a try, please? It removes the SEGFAULT for me, but then this being a Heisenberg type bug, this may be accidental. -- Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2832/possible-race-con…

1 0

#2832: possible race condition in LoopControl
by Roland Haas 05 Nov '24

05 Nov '24

#2832: possible race condition in LoopControl Reporter: Roland Haas Status: new Milestone: Version: Type: bug Priority: major Component: Comment (by Roland Haas): Applied as git hash [ff99c0d16](https://github.com/einsteintoolkit/Carpet/commits/ff99c0d16feb62… "LoopControl: replace **allign** with posix\_memalign" of [Carpet](https://github.com/einsteintoolkit/Carpet) -- Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2832/possible-race-con…

1 0

#2832: possible race condition in LoopControl
by Roland Haas 05 Nov '24

05 Nov '24

#2832: possible race condition in LoopControl Reporter: Roland Haas Status: new Milestone: Version: Type: bug Priority: major Component: Comment (by Roland Haas): Like so: [https://github.com/EinsteinToolkit/Carpet/pull/11](https://github.com/Einst… ? -- Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2832/possible-race-con…

1 0

#2832: possible race condition in LoopControl
by Erik Schnetter 05 Nov '24

05 Nov '24

#2832: possible race condition in LoopControl Reporter: Roland Haas Status: new Milestone: Version: Type: bug Priority: major Component: Comment (by Erik Schnetter): We don’t need `new` or `std::vector`. We replace the call to `new` with `posix_memalign`, and the call to `delete` by `free`. Then each thread has one cache line as intended. Alternatively, there could be one call to `posix_memalign` allocating memory for all threads, and each thread uses pointer arithmetic to find its place. -- Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2832/possible-race-con…

1 0

#2832: possible race condition in LoopControl
by Roland Haas 05 Nov '24

05 Nov '24

#2832: possible race condition in LoopControl Reporter: Roland Haas Status: new Milestone: Version: Type: bug Priority: major Component: Comment (by Roland Haas): Minor point: Padding size wise I think just adding and _extra_ 128 bytes \(or whatever the cache line size is\) instead of padding to 256 \(twice the cache line size\) should be sufficient to ensure that there are no shared cache lines since even in the worst case scenario where the first structure member is a the very end of a cache line padding by a full cache line size skips into the next cache line for the next array member \(which will start somewhere near the beginning of its cache line of then\). `posix_memalign` might be more efficient. Would still need a comment like right now since we do not want alignment but separation. -- Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2832/possible-race-con…

1 0

#2832: possible race condition in LoopControl
by Roland Haas 05 Nov '24

05 Nov '24

#2832: possible race condition in LoopControl Reporter: Roland Haas Status: new Milestone: Version: Type: bug Priority: major Component: Comment (by Roland Haas): Right, I was thinking of it but did not consider it as likely to pass the smell test. Namely: how to nicely combine that one with `std::vector`? Works fine to replace the naked `new` of course. For the `std::vector` one would use something like this: ``` lc_fine_thread_comm_t lc_fine_thread_comm = nulltpr; [...] if (!lc_fine_thread_comm) { lc_fine_thread_comm = static_cast<lc_fine_thread_comm*>(posix_memalign(n*sizeof(*lc_fine_thread_comm), 128)); for(size_t i = 0 ; i < n ; ++i) { lc_fine_thread_comm_t* ptr = new (lc_fine_thread_comm+i) lc_fine_thread_comm_t(); assert(ptr == lc_fine_thread_comm+i); // apparently not guaranteed } } ``` and similar awkwardness if we ever were to `free` the array. -- Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2832/possible-race-con…

1 0

#2832: possible race condition in LoopControl
by Erik Schnetter 05 Nov '24

05 Nov '24

#2832: possible race condition in LoopControl Reporter: Roland Haas Status: new Milestone: Version: Type: bug Priority: major Component: Comment (by Erik Schnetter): Ah yes, silly me. We should be using `posix_memalign`. -- Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2832/possible-race-con…

1 0

#2832: possible race condition in LoopControl
by Roland Haas 05 Nov '24

05 Nov '24

#2832: possible race condition in LoopControl Reporter: Roland Haas Status: new Milestone: Version: Type: bug Priority: major Component: Comment (by Roland Haas): There’s `alignas` since C\+\+11: [https://en.cppreference.com/w/cpp/language/alignas](https://en.cppreference… though this may not be any different from `CCTK_ATTRIBUTE_ALIGN` and indeed ``` #include <vector> struct alignas(128) aligned_t { volatile int foo; }; std::vector<aligned_t> test_aligned_vector; aligned_t *aligned; void foo() { aligned = new aligned_t(); } ``` gives the same warning when compiled with `g++ -std=gnu++11 -c -Wall` \(only warns about `new` not about `std::vector` but I suspect that `std::vector` also does not align and only hides the issue because it internally uses a `void*` pointer for its storage\). -- Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2832/possible-race-con…

1 0

#2832: possible race condition in LoopControl
by Roland Haas 05 Nov '24

05 Nov '24

#2832: possible race condition in LoopControl Reporter: Roland Haas Status: new Milestone: Version: Type: bug Priority: major Component: Comment (by Roland Haas): Oh, right. So that won’t quite prevent cache line sharing. Unfortunately I cannot check alignment with a static assert since both types are allocated dynamically \(`lc_thread_info_t` via a `new` the other as part of a `std::vector<lc_fine_thread_comm_t>` \(and I’d have no idea how that one would have interacted with the `CCTK_ATTRIBUTE_ALIGNED(128)`\). So I guess it would have to be using 2\*128 or at least 2 times the cache line size if that one is know at compile time. -- Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2832/possible-race-con…

1 0

#2832: possible race condition in LoopControl
by Erik Schnetter 05 Nov '24

05 Nov '24

#2832: possible race condition in LoopControl Reporter: Roland Haas Status: new Milestone: Version: Type: bug Priority: major Component: Comment (by Erik Schnetter): As you say, your padding doesn’t actually align the struct. The struct may thus be split across two cache lines. Since some architectures have 128-byte cache lines, you should instead pad to 2\*128 bytes. \(Intel CPUs have 64-byte cache lines and thus 128 bytes suffice there.\) You can instead also check the alignment \(see [https://en.cppreference.com/w/c/language/\_Alignof](https://en.cppreference…) with a static assertion. -- Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2832/possible-race-con…

1 0

← Newer
1
...
5
6
7
8
9
10
11
Older →

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

Trac November 2024