LINUX.ORG.RU

Видите ли вы в коде какие-нибудь проблемы?

 ,


0

2

Всем привет. Локализовал код, на который жалуется санитайзер. Не вижу здесь своей ошибки, а вы?

#include <thread>
#include <atomic>
#include <mutex>
#include <vector>
#include <chrono>
using namespace std;

class Test {
	mutex m_mtx;
	atomic_flag m_spin_lock;
	vector<int> m_data;
public:
	void add() {
		while (m_spin_lock.test_and_set(memory_order_relaxed))
			this_thread::yield();
		atomic_thread_fence(memory_order_acquire);
		while (true) {
			this_thread::sleep_for(300ms);
			std::scoped_lock lck{m_mtx};
			m_data.push_back(4);
		}
		m_spin_lock.clear(memory_order_release);
	}
	void read() {
		size_t sz;
		while (true) {
			if (! m_spin_lock.test_and_set(std::memory_order_acquire)) {
				sz = m_data.size();
				m_spin_lock.clear(std::memory_order_release);
			}
			else {
				std::scoped_lock lck{m_mtx};
				sz = m_data.size();
			}
			this_thread::sleep_for(1s);
		}
	}
}test;

int main() {
	jthread rt(&Test::read, &test);
	this_thread::sleep_for(10ms);
	test.add();
}
$ g++ 1.cc -std=c++20 -fsanitize=thread
$ ./a.out
WARNING: ThreadSanitizer: data race (pid=63269)
  Write of size 8 at 0x56247be53a80 by main thread (mutexes: write M11):
    #0 void std::vector<int, std::allocator<int> >::_M_realloc_insert<int>(__gnu_cxx::__normal_iterator<int*, std::vector<int, std::allocator<int> > >, int&&) <null> (a.out+0xd9a47)
    #1 int& std::vector<int, std::allocator<int> >::emplace_back<int>(int&&) <null> (a.out+0xd970c)
    #2 std::vector<int, std::allocator<int> >::push_back(int&&) <null> (a.out+0xd9595)
    #3 Test::add() <null> (a.out+0xd819b)
    ...

  Previous read of size 8 at 0x56247be53a80 by thread T1:
    #0 std::vector<int, std::allocator<int> >::size() const <null> (a.out+0xd899d)
    #1 Test::read() <null> (a.out+0xd7bc2)
    ...

Со шлангом аналогично.

★★

Последнее исправление: kvpfs (всего исправлений: 1)

Проблема в 14 строке.

P.S. объясните мне кто-нибудь что это такое он запостил?

bhfq ★★★★★
()
Ответ на: комментарий от bhfq

Проблема в 14 строке.

Так после ведь aquire барьер стоит, вроде ок.

P.S. объясните мне кто-нибудь что это такое он запостил?

Логика примерно такая: есть разделяемый данные в объекте, есть служебный поток, который сидит внутри и поддерживает его «живым». Другие потоки обращаются к объекту со всякими запросами и если могут взять spinlock (случай, когда служебного потока в объекте нет), то сами делают нужную работы (модификация или копирование чего-то), если же spinlock не взят (внутри есть служебный поток), то берётся мьютекс, ставится задание в очередь и ожидается обработка запроса.

kvpfs ★★
() автор топика
Последнее исправление: kvpfs (всего исправлений: 1)
Ответ на: комментарий от bhfq

Проблема в 14 строке.

В 19 и 28/33.

что это такое он запостил?

Шизу пятиклассника, познающего мир методом брутфорса. Очевидно же.

LamerOk ★★★★★
()
Ответ на: комментарий от jo_b1ack

Это для теста, нафиг усложнять всякими флагами и прочим код, который возможно пойдёт в багрепорт.

kvpfs ★★
() автор топика
Ответ на: комментарий от kvpfs

Это для теста, нафиг усложнять всякими флагами и прочим код, который возможно пойдёт в багрепорт.

А так задумано, что sleep выполняется с захваченым мьютексом?

BRE ★★
()
Ответ на: комментарий от kvpfs

спс, поправил.

вы там все поправьте.

while (m_spin_lock.test_and_set(memory_order_relaxed))
			this_thread::yield();

тут тред крутится пока флаг не сбросят, фактически в лупе. причем если переключаться не на кого, тред даже не переключится.

йелды это для сопрограмм, в нормальном мултитреде их использование говорит о том, то тут какая-то порочная логика и чувак явно путаный. йелдами занимаются обьекты синхронизации, коотрые вы должны использовать.

такое впечатление, что вы все, что прочитали свалили в одну кучу,не пойми зачем.

alysnix ★★★
()

и еще совет - не надо в коде спинлокать, барьерить и йелдить.

это говорит о том, что вы изобретаете какой-то свой уникальный обьект синхронизации, скорее всего сделаете его криво, и вы еще должны доказать миру, что такой обьект необходим и не покрывается стандартными мьютексами, критсекциями и семафорами. потому что своим кодом вы утверждаете, что вам этих стандартных обьектов не хватает.

опять же такой обьект надо отдельно тогда реализовать, с четкой семантикой и использовать его, а не спинлоки по коду распихивать.

alysnix ★★★
()
Последнее исправление: alysnix (всего исправлений: 1)
@@ -25,6 +25,7 @@
                size_t sz;
                while (true) {
                        if (! m_spin_lock.test_and_set(std::memory_order_acquire)) {
+                               std::scoped_lock lck{m_mtx};
                                sz = m_data.size();
                                m_spin_lock.clear(std::memory_order_release);
                        }
anonymous
()
Ответ на: комментарий от alysnix

тут тред крутится пока флаг не сбросят, фактически в лупе. причем если переключаться не на кого, тред даже не переключится.

Ничего криминального в этом нет, в задаче спинлок берётся в цикле лишь служебным потоком, в норме это происходит лишь однажды после создания объекта. Но чтобы работало и во всяких маргинальных кейсах - сделал так, допустимо, думаю.

kvpfs ★★
() автор топика
Ответ на: комментарий от anonymous

Давайте с аргументами. Почему ты считаешь, что мой код кривой, а твоя правка делает его валидным? В add() я всегда пушаю со взятым спинлоком и мьютексом, следовательно, в read() я могу синхронизироваться через любой из них (мьютекс или спинлок).

kvpfs ★★
() автор топика
Последнее исправление: kvpfs (всего исправлений: 1)
Ответ на: комментарий от kvpfs

Ничего криминального в этом нет

полинг всегда криминален. уж сколько об этом написано статей и советов по всему инету.

треды управляются событиями!!! и встают на ожидание!!! а не крутятся в опросах каких-то флагов. полинг применим только в очень коротких циклах, обычно со счетчиком цикла поллинга(поопрашивал раз 20 и выскочил с неуспехом), а не от балды, пока кто-то флаг не сбросит… а если он его вообще не сбросит никогда?

alysnix ★★★
()
Последнее исправление: alysnix (всего исправлений: 1)
Ответ на: комментарий от kvpfs

хз, что ты от кода хочешь, правка удовлетворяет sanitize=thread

anonymous
()

вы не шаред мьютекс там изобретаете? задача непонятна

https://en.cppreference.com/w/cpp/thread/shared_mutex

вот если ваш мастерный поток будет брать эксклюзивный лок, а слейвы - разделенный, то …если они конечно будут только читать, не меняя обьект - то оно работать будет. для того и сделано. это ваш случай?

alysnix ★★★
()
Последнее исправление: alysnix (всего исправлений: 2)
Ответ на: комментарий от alysnix

вы не шаред мьютекс там изобретаете?

Не, в данном случае можно сделать на обычном нешаренном мьютексе. служебный поток (который входит через соответствующую функцию) берёт мьютекс, а остальные пробуют try_lock() и в случае неудачи идут ставить запрос в очередь для служебного потока.

kvpfs ★★
() автор топика
Ответ на: комментарий от Siborgium

Тем, что это даже просто быстрее (хотя я это вряд ли замечу, но …). В обычном ходе выполнения потоки не будут попадать на поллинг, лишь протестили atomic_flag, не получилось, пошли ствить заявку в очередь.

kvpfs ★★
() автор топика
Ответ на: комментарий от kvpfs

Я сейчас штудирую букварь, чтобы получить какое-то формальное подтверждение своей правоты, а не на уровне интуиции.

kvpfs ★★
() автор топика

ну такой код точно в линукс не примут — давай переделывай все как надо

anonymous
()

Дело вот в чём:

The thread sanitizer (-fsanitize=thread) does not handle C++ atomic_thread_fence. This is barely acknowledged in the documentation, but causes a number of users to waste a lot of time trying to understand how the reported race could occur. Ideally, it would be supported, but that seems hard. What does seem doable is adding a warning for programs using fences with tsan. I don't really care if it is a compile-time warning, or a runtime warning, and in the second case whether it appears as soon as a fence is executed or only as a note after reported races, as long as I get a hint about it.

atomic_thread_fence барьеры санитайзер просто не может. Ну и если вместо while (m_spin_lock.test_and_set(memory_order_relaxed)) вписать while (m_spin_lock.test_and_set(memory_order_acquire)), то всё норм. TSan очень сырой, уже второй раз спотыкаюсь об него и трачу кучу времени на попытку понять несуществующую проблему в своём коде.

kvpfs ★★
() автор топика
Ответ на: комментарий от kvpfs

Дело вот в чём:

понять несуществующую проблему в своём коде.

Дело в том, что ты пытаешься

получить какое-то формальное подтверждение своей правоты

А тебе нужно искать исключительно подтверждение твоей неправоты.

Siborgium ★★★★★
()

по простому тебе нужен только std::lock_guard<mutex> на операцию модификации контейнера и все

anonymous2 ★★★★★
()
Последнее исправление: anonymous2 (всего исправлений: 1)

Ничего ужасного нет в spinlock’ах, если руки прямые. Я не собираюсь крутить бесконечные поллинг циклы, я ведь писал, единственное что надо - сделать try_lock() для определения факта наличия служебного потока в недрах объекта. А служебный поток въезжает в норме однажды после создания (да, и там есть поллинг цикл, но лишь однажды и не долго), вот типичный кейс, именно под него и сделал spinlock.

Можно сравнить две реализации тестого хелоу ворлда (изменив лишь способ синхронизации) - одна на мьютексе, другая на спинлоке:

#include <mutex>
#include <atomic>
#include <thread>
#include <iostream>
using namespace std;

unsigned sh_data;
mutex mtx;
atomic_flag flag;

void f() {
	for (unsigned i = 0;  i < 50'000'000;  ++ i)
#ifdef USE_ATOMIC
		if (! flag.test_and_set(std::memory_order_relaxed)) {
			atomic_thread_fence(memory_order_acquire);
			++ sh_data;
			flag.clear(std::memory_order_release);
		}
#else
		if (mtx.try_lock()) {
			++ sh_data;
			mtx.unlock();
		}
#endif
}

int main() {
	if (true) {
		jthread t(f);
		f();
	}
	cout << sh_data << endl;
}
$ g++ 3.cc -O2 -pthread -std=c++20  &&  time ./a.out 
35310437

real    0m5.803s
user    0m11.439s
sys     0m0.002s
$ g++ 3.cc -O2 -pthread -std=c++20 -DUSE_ATOMIC  &&  time ./a.out 
50394575

real    0m2.933s
user    0m5.849s
sys     0m0.001s

И разница даже не на каком-то weakly-ordered cpu, а выйгрыш скорее от отсутствия вызовов в pthread. Но разница заметна, зачем мне брать более тормозной вариант? Не вижу смысла.

kvpfs ★★
() автор топика
Ответ на: комментарий от alysnix

елды это для сопрограмм, в нормальном мултитреде их использование говорит о том, то тут какая-то порочная логика и чувак явно путаный. йелдами занимаются обьекты синхронизации, коотрые вы должны использовать.

std::this_thread::yield — Provides a hint to the implementation to reschedule the execution of threads, allowing other threads to run.
что то я не вижу в таком использовании проблему — что бы потоковый шедулер начал выполнять другие треды пока тут не выполнилось условие... — иначе — будет просто проработка впустую для проца всегда.

safocl ★★
()
Ответ на: комментарий от alysnix

ну может ему не надо доказывать ничего — вполне такое может быть.

на счет примитивов синхронизации — их есть множество — и это множество показывает, что не все они универсальны.

+ на счет реализации отдельно.

safocl ★★
()
Ответ на: комментарий от alysnix

и встают на ожидание!!! а не крутятся в опросах каких-то флагов.

что-то противоречивое — как же по твоему они ожидают без опроса каких-то флагов? — магией?

safocl ★★
()
Ответ на: комментарий от safocl

что-то противоречивое — как же по твоему они ожидают без опроса каких-то флагов? — магией?

причем тут магия. когда тред приходит на обьект синхронизации и ввиду неуспеха должен ждать, это самое ожидание, есть просто приостановка данного треда, помещение его дескриптора в очередь этого ждущих на данном обьекте (это список ожидающих дескрипторов тредов), и переключение на другой тред.

с этого момента тред находится в «ожидании». то есть пока по той или иной причине тред не будет перемещен из этой очереди в очередь активных на планировщике. то есть тред спит «хррр хррр» самым настоящим образом и никоим образом не будет исполняться, пока спит.

а «опрос флагов» делает как раз тред, что активен.

то есть - «находиться в ожидании» означает - не быть в списке активных тредов планировщика, а в какой-то очереди, откуда по некоему событию его извлекут и «разбудят» - то есть поместят в список активных.

alysnix ★★★
()
Последнее исправление: alysnix (всего исправлений: 2)
Ответ на: комментарий от alysnix

так будет запрос каких то флагов или нет? что-то же будет проверяться каждый раз на треде по коду — надо выполниться или же дальше простаивать — или же это будет какой то магией противоестественной делаться?

safocl ★★
()
Ответ на: комментарий от alysnix

и да — так если тред выделяется специально для этих целей — то какая вообще разница, если он спит постоянно? он кого не толкает и не притесняет...

safocl ★★
()
Вы не можете добавлять комментарии в эту тему. Тема перемещена в архив.