Aplikacja wielowątkowa, sesje krytyczne

0

Witam.

W aplikacji wielowątkowej:

  • W jednym wątku mam pętle, która pobiera zdarzenia

// Watek 1

while(true)
{
	THREAD_LOCK(events->lock);

	if(events->GetEvent(&event,THREAD_MAIN))
	{
		switch(event->getEventType())
		{
			case COS_SIE_DZIEJE:

			
			break;
		}
	}

	THREAD_UNLOCK(events->lock);

}

Tak samo jest tez w drugim wątku


// Watek 2

while(true)
{
	THREAD_LOCK(events->lock);

	if(events->GetEvent(&event,THREAD_MAIN))
	{
		switch(event->getEventType())
		{
			case COS_SIE_DZIEJE:

			
			break;
		}
	}

	THREAD_UNLOCK(events->lock);

}

Lecz w 3 wątku są funkcje które są wywoływane i dodawane są zdarzenia:


// 3 Wątek

// Przykładowa funkcja ktora dodaje zdarzenie

void EventSystem::onCosSieDzieje()
{
       // o tego locka będzie chodziło w dalszym tekscie
	THREAD_LOCK(lock);

	Event* event = new Event(COS_SIE_DZIEJE);

	AddEvent(event);

        // i tego unlocka
	THREAD_UNLOCK(lock);
}

I jest problem...

w funkcji OnCosSieDzieje jest lock którego wyróżniłem komentarzem kiedy go usune to program działa jak należy, ale jeśli nie to po kilkunastu sekundach działania programu program się deadlockuje.

Inaczej jest jeżeli w pętli nieskonczonej pomiędzy pobraniem zdarzeń w wątku nr 1 i w wątku nr 2 dodam Sleep, np Sleep(1), wtedy program deadlockuje się ale po kilkunastu lub kilku minutach działania programu.

W programie nie występują inne locki events->lock, które mogłyby wywołać deadlocka...

Czy możliwe, że np coś takiego:


while(true)
{
	THREAD_LOCK(events->lock);

	THREAD_UNLOCK(events->lock);
}

ze względu na brak uśpienia wątku (Sleep)
jest traktowane jako jeden długi lock, i dlatego jest deadlock?

ale przecieŻ nawet gdy dam Sleep(1), to deadlock wystepuje ale dopiero po kilku, kilkunastu mintutach

PS. Obiekt events to globalny singleton klasy EventSystem

więc...

Co się dzieje?

Z góry dziękuje za pomoc

0

A dzieje się to, że każdy wątek zakłada lock'a praktycznie na cały czas.
Załóż locka, zdejmij event z kolejki i od razu odblokuj lock'a, dopiero później wykonuj "obliczenia".

PS. wypadałoby napisać z jakiej biblioteki korzystasz.

0

Hm, z żadnej biblioteki nie korzystam. THREAD_LOCK to makro do EnterCriticalSection, a funkcja GetEvent to moja funkcja.

0

Tylko, że ten system eventów, zdejmuje i usuwa eventa dopiero jak wszystkie "pobieracze eventów" go przeskanują. Tzn jest kilka wątków pobierających eventy, i każdy ma swój id. przy pobieraniu eventa - zaznacza tam swój id i jeżeli event jest pobierany przez ostatniego pobieracza eventów to jest zdejmowany z kolejki.

I właśnie jest problem, bo jezęli locka zasadze tylko na pobranie eventa, to 2 pobieracze mogą operować na nim jednoczesnie.

W sumie dlaczego niby aplikacja się deadlockuje na zawsze? przecieŻ gdyby tak było jak napisałeś to chyba by każdy wątek przeskanował te zdarzenia i by się odblokowało, a tak to deadlock jest cały czas ... : /

Nie mam pojęcia co jest źle.

Proszę o pomoc, z góry dziękuje

0

No cóż, musisz podać pełny kod w takim razie. Tzn. to co istotne czyli budowa kolejki i ciało wątku.

I właśnie jest problem, bo jezęli locka zasadze tylko na pobranie eventa, to 2 pobieracze mogą operować na nim jednoczesnie.
No właśnie kluczowym jest co te wątki robią z eventami i jak.

W tym kodzie co masz teraz w jednym czasie pracuje tylko 1 wątek ! Nie wiem dlaczego jest deadlock, musiałbym mieć pełny kod, możesz to łatwo sprawdzić dobrym debuggerem.
Może jeśli kolejka jest pusta wątek chcąc poprać event czeka aż ten nadejdzie, ale nie zwalnia locka przez co nowy event nie może zostać dodany? Poczytaj o problemie producent-konsument. Potrzebna jest druga sekcja krytyczna. Pisałem nawet o tym ostatnio w innym wątku.

Piszesz, że każdy event ma być przerobiony przez każdy z wątków roboczych ale tylko przez jeden wątek w jednym czasie. Tak więc zbuduj kolejkę w następujący sposób.
Na wstępie proponuje trzymać sekcje krytyczną w klasie kolejki, a locka zakładać niejawnie wewnątrz metod kolejki, np:

void EventQueue::AddEvent(Event* event)
{
   lock(this->cs);
   this->Add(event);
   unlock(this->cs);
}

po prostu oprogramować kolejkę tak aby była thread-safe.

Z każdym eventem powiązać: listę ID wątków które go przerobiły oraz ID wątku, który aktualnie na nim pracuje.
Dodać do kolejki dwie metody:
Event *EventQueue::BeginWork(int ID) - przydziela pierwszy wolny event wątkowi o danym ID, który nie był jeszcze przez niego przerabiany i oznacza go jako zajęty danym wątkiem.
void EventQueue::EndWork(int ID) - zwalnia event na którym pracował dany wątek, oznacza go jako przerobiony danym wątkiem i jeśli został już przez wszystkie wątki przerobiony usuwa z kolejki.

0

mi to nie wyglada na deadlock sensu stricte, ale raczej na zaglodzenie - ktorys z watkow dochodzi bardzo czesto do glosu, pewnie robotnicy na zmiane sa przelaczane miedzy soba, a watek 3ci w ogole nie dochodzi do glosu -- lock jest spuszczony na ulamek sekundy, nie wiadomo tez jak system kolejkuje watki na lockach.
przemawia za tym i konstrukcja (zwalnianie tylko na momencik, dwoch robotnikow z while-true) oraz zachowanie (sleep wstrzelony wydluzyl czas wilnego locka i radykalnie zmienil zachowanie 'systemu'!)

rozwiazanie szybkie:

  • tworzac watki - nadaj watkom 1) i 2) priority NIZSZE niz 3) - w ten sposob zagwarantujesz, ze producent eventow bedzie mial szanse w ogole dodawac.. w zasadzie, kiedy tylko piszesz jakikolwiek watek z while(true) warto sie zastanowic czy aby nie ma on chodzic na priority jakims najnizszym ;P ale -- ryzykujesz, ze jesli eventy beda BARDZO czesto przychodzic, to z kolei zaglodzisz robotnikow (odbiorcow eventow)
  • przebudowac kod, tak aby zamiast puszczania locka na momencik - tylko na momencik go zabieral. tak w zasadzie powinno sie tworzyc wszelkie mechanizmy ktore wspoldziela zasoby.. idea w skrocie:
     producent:
           onCostam()
                  tmp = createNewItem()
                  lock.hold()
                  queue->Add(tmp)
                  lock.release()
     konsument:
           while(...)
                  lock.hold()
                  if(!queue->hasItems())
                      lock.release()   // nie ma? to nic nie rob
                      yield()   // skoro nic nie ma, to oddaj czas procesora. nie ma yield? uzyj sleep(0)
                  else
                      tmp = queue->Get()
                      queue->Remove(tmp)   // ISTOTNE. dzieki temu 2 watki nie obsluza tego samego
                      lock.release()  // ISTOTNE. zwolnij PRZED rozpoczeciem wlasciwej pracy
                      ..zrob cos z tmp..
0
quetzalcoatl napisał(a)

mi to nie wyglada na deadlock sensu stricte, ale raczej na zaglodzenie
Czyli to o czym pisałem

adf88 napisał(a)

Może jeśli kolejka jest pusta wątek chcąc poprać event czeka aż ten nadejdzie, ale nie zwalnia locka przez co nowy event nie może zostać dodany

Trzeba wspomnieć, że twoje rozwiązanie ma pewną wadę (możliwe, że nie istotną) - w przypadku zagłodzenia wątki będą zabierać czas procesora pÓÓÓÓki nie pojawi się nowy event. Można by dać dłuższego sleep'a ale znowuż będzie powodować to marnotrawienie czasu gdyż event może się pojawić, gdy wątek śpi (też możliwe że nie będzie to istotne). Tak więc dobranie długości sleep'a reguluje kompromis pomiędzy marnotrawieniem czasu a niepotrzebnym wykorzystaniem procesora. Jeśli te zasoby są krytyczne to pełną wydajność osiągniesz dzięki zastosowaniu drugiej sekcji krytycznej, czy też innego mechanizmu synchronizacji.

0

Właśnie trochę przebudowywuje kod (tzn skonczylem ale jest jakis blad - debuger go wykrywa w vector push_back, musze go naprawic)

A co do tego, że deadlock jest bo GetEvent czeka na eventa to tak nie jest na pewno.
Jak GetEvent wykryje ze w kolejce nie ma eventa to zwraca false.

Po przebudowaniu kodu wygląda to mniej więcej tak:

lock jest zakladany zdejmowany Od razu w GetEvent


while(true)
{
		if(events->GetEvent(&event,GADBOT_THREAD_MAIN))
		{
			events->BeginWorking(event);

			switch(event->getEventType())
			{
				case COS_SIE_DZIEJE:
                               
                               
                                break;
			}

			events->EndWorking(event);
		}

               Sleep(1);
}

ale mysle tez zeby zrobic tak:


while(true)
{
		while(events->GetEvent(&event,GADBOT_THREAD_MAIN))
		{
			events->BeginWorking(event);

			switch(event->getEventType())
			{
				case COS_SIE_DZIEJE:
                               
                               
                                break;
			}

			events->EndWorking(event);
		}

               Sleep(5);
}
0

No i super. Wydaje mi się, że możesz się pokusić nawet o dłuższego sleep'a. Zresztą zrób test - nie podawaj event'ów i sprawdź jakie jest wykorzystanie procesora w zależności od długości sleep'a.
A jeśli zagłodzenie nie występuje często ani długo to raczej nie przejmuj się zbytnio i zostaw np. to 5 ms.

0

Chyba 5 ms wystarczy, bo wtedy prawie nie zużywa procesora.

Tylko właśnie coś tego błędu nie moge naprawić.

W funkcji GetEvent jest właśnie błąd.

Każdy wątek (pobieracz eventów) w funkcji GetEvent jak pobierze eventa to wywołuje funkcje

event->checkedBy(threadId)

I Każdy event ma liste właśnie przez kogo był sprawdzany.

I w funkcji checkedBy dodaje sie do tej listy(std::vector) za pomocą push_back

I właśnie debuger pokazuje tam błąd (głębiej pokazuje w vector w push_back i w insert)

std::vector<unsigned int,std::allocator<unsigned int="int"> >::_Insert_n(std::_Vector_const_iterator<unsigned int,std::allocator<unsigned int="int"> > _Where=..., const unsigned int & _Val=6400, unsigned int _Tmp=68812648) Line 1178 + 0x9 bytes

I jeżeli zostawiłem tylko jeden wątek to działało wszystko - dopiero jak 2 pobieracze działają to wtedy jest ten błąd.

0

Każdy wątek (pobieracz eventów) w funkcji GetEvent jak pobierze eventa to wywołuje funkcje

event->checkedBy(threadId)
To też trzeba synchronizować.
Generalnie radziłbym odseparować informację o tym kto już przerobił event od zarówno wątków jak i klasy Event. Niech klasa kolejki się tym zajmuje, niech to będą jej prywatne mechanizmy. Wtedy nie powinny się pojawić takie problemy j.w.
Oznaczaj event jako przerobiony w EndWorking wewnątrz lock'a.

0

Tylko, że jak mam trzymać informacje jaki event był sprawdzany przez danego pobieracza?

Bo tak to każdy event ma swoją liste(vector) i bardzo mało czasu zajmuje jej przeszukanie bo tam może być max kilka elementów (bo tylko kilka jest pobieraczy).

A jak by w klasie kolejki miała być np lista struktur

struct EventChecked
{
Event* event,
int checkerId
}

to by tych elementów było po kilkaset i przeszukanie tego by dużo czasu zajęło.

I jeszcze trzeba by było to przeszukać do usunięcia.

A zdarzeń przychodzących mam bardzo dużo (nawet do kilkunastu na sekunde)

0

Możesz zrobić tak:

class EventQueue
{
private:
   int num_workers; //ilość wątków roboczych

   struct EventInfo
   {
      Event* event;
      unsigned workDone; //które wątki już zrobiły swoje (maska bitowa)
      bool working; //czy aktualnie event jest wykorzystywany
   };

   list<EventInfo> events; //kolejka właściwa zdarzeń

   vector<list<EventInfo>::iterator> currentWork; //na których eventach pracują wątki

public:
   int EventQueue::CommitWorker() //przydzielamy identyfikator wątkowi roboczemu
   {
      lock();
      num_workers++;
      currentWork.resize(num_workers);
      int ret = num_workers;
      unlock();
      return ret;
   }

   Event* BeginWorking(int worker)
   {
      lock();
      list<EventInfo>::iterator result = FindFirstFreeNotDone(worker); //wyszukiwanie pierwszego wolnego, nieprzerobionego eventu
      if (result != events.end()) result->working = true;
      unlock();
      currentWork[worker] = result;
      return (result == events.end()) ? NULL : result->event;
   }

   void EndWorking(int worker)
   {
      list<EventInfo>::iterator doneEvent = currentWork[worker]; //na którym evencie pracował wątek
      lock();
      doneEvent->workDone |= (1 << worker); //dodajemy bit zakończenia pracy
      if (doneEvent->workDone == ((1 << num_workers) - 1)) //jeśli wszystkie wątki przerobiły ten event
         events.remove(doneEvent); //to usuwamy go z listy
      unlock();
   }

};

//watek:
int myId = events.CommitWorker();
Sync(); //synchronizacja z pozostałymi wątkami, czekamy aż wszystkie ID zostaną przydzielone
while(true)
{
   Event *event = events->BeginWorking(myId);
   if (event == NULL)
   {
      Sleep(5);
   }
   else
   {

      //pracujemy...

      events->EndWorking(myId);
   }
}

proponuje coś w ten deseń

Wszystkie operacje O(1), oprócz wyszukiwania wolnego eventa

Przydzielanie identyfikatorów wątkom powinno się raczej zrobić przed ich wystartowaniem (niepotrzebna będzie synchronizacja), ale już nie chciało mi się tego pisać.

0

Trochę stary temat : p, ale w tym samym dniu od ostatniej odpowiedzi adf88 wyjechałem 10 tys km z tąd na 1.5 miesiąca i zapomniałem o tym, a wypadałoby jakoś zakończyć ten temat bo będe kontynuował pisanie tego programu.

Btw. Od razu dzięki wielkie za kod : P

void EndWorking(int worker)
{
      EventInfo* doneEvent = currentWork[worker]; //na którym evencie pracował wątek
      lock();
      doneEvent->workDone |= (1 << worker); //dodajemy bit zakończenia pracy
      if (doneEvent->workDone && ((1 << num_workers) - 1)) //jeśli wszystkie wątki przerobiły ten event
         events.remove(doneEvent); //to usuwamy go z listy
      unlock();
 }

i nie rozumiem jednej rzeczy

if (doneEvent->workDone && ((1 << num_workers) - 1)) //jeśli wszystkie wątki przerobiły ten event

Czy zamiast && nie powinno być == ?

i, jak to z tym usuwaniem obiektów zdarzen? robic jakies oddzielna część czy tutaj usuwac:

      if (doneEvent->workDone && ((1 << num_workers) - 1))
      { 
         events.remove(doneEvent); //to usuwamy go z listy
         delete doneEvent;
      }
0

Faktycznie trochę w tej metodzie błędów było. Już poprawiłem.
Zamiast "&&" ma być oczywiście "==".
Natomiast "delete" już nie, obiekty EventInfo trzymane są na liście przez wartość, nie są one tworzone dynamicznie więc i usuwać nie musimy.
Jeszcze zamiast iteratora omyłkowo był wskaźnik.

1 użytkowników online, w tym zalogowanych: 0, gości: 1