Спин блокировка - RTFM ?

[resource]

Многострадальный passthru. В топике NDIS Firewall была дана ссылка на Extending passthru 2. Код, о котором пойдет речь далее из этой серии. Но вопрос в этот раз не о фильтрации траффика или каких-то примудростях NDIS. Представляю кусок функции перечилсяющей приатаченные PADAPT'ы (на языке passthru)

Код:

	pListEntry = pListHead;

	NdisAcquireSpinLick(&Globals.GlobalLock);

	while (pListEntry->Flink != pListHead)
	{	
		pListEntry = pListEntry->Flink;
		pAdapt = (PADAPT)pListEntry;

		RtlCopyMemory(pBuffer,
				pAdapt->DeviceName.Buffer,
				pAdapt->DeviceName.Length);

		pBuffer +=  pAdapt->DeviceName.Length;
		*((PWCHAR )ioBuffer) = UNICODE_NULL;
		pBuffer += sizeof(UNICODE_NULL);

		RtlCopyMemory(pBuffer,
				pAdapt->LowerDeviceName.Buffer,
				pAdapt->LowerDeviceName.Length);

		pBuffer +=  pAdapt->LowerDeviceName.Length;
		*((PWCHAR )ioBuffer) = UNICODE_NULL;
		pBuffer += sizeof(UNICODE_NULL);		
	}

	NdisReleaseSpinLock(&Globals.GlobalLock);

Насчет того, что

Код:

		pAdapt = (PADAPT)pListEntry;

...я всё могу объяснить....

Код:

typedef struct _ADAPT
{
	LIST_ENTRY			List;
.....
} ADAPT, *PADAPT;

Но, собственно, вопрос не в этом. Вопрос в захвате спин блокировки перед циклом, и освобождением после него. Всем известно, что спин блокировку нужно удерживать как можно меньшее время, т.е. освобождать как можно быстрее. DDK говорит что 25 микросекунд это "смертельный" предел (да ктож их возьмется подсчитать в многозадачной ОС). Ну а что если в списке будет 100 (чисто гипотетически) структур PADAPT. Нехорошо как получается - надеиться что всё успеется. Вопрос: действительно ли это так нехорошо как кажется? И какие тут еще есть варианты, более правильные и безопасные?

[Ochkarik]

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

и я не совсем въехал в то, что собственно в цикле происходит?
*((PWCHAR )ioBuffer) = UNICODE_NULL; - тут наверное опечатка? должно быть pBuffer?

еще... тут у вас есть нюанс с добавлением/удалением из другой нити. в частности вызов  pListEntry = pListEntry->Flink; не очень хороший... (если не блокировать весь список целиком)
были функции типа NdisInterlockedPopEntrySList/NdisInterlockedInsertHeadList.
может быть имеет смысл построить на них?)

что может происходить с записями списка? только удаление/добавление или модификация тоже?
кстати еще есть забавная NdisInitializeReadWriteLock

[resource]

*((PWCHAR )ioBuffer) = UNICODE_NULL; - тут наверное опечатка? должно быть pBuffer?

Совершенно верно. Опечатка.

с добавлением/удалением из другой нити

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

не очень хороший... (если не блокировать весь список целиком)

Ну а что делать.

может быть можно блокировку внутрь цикла внести?)

Я тоже сразу такое подумал, но получалось всё как-то некрасиво. Как сделаю вариант, представлю вам.
Кстати Томас Дивайн (с PCAUSA) в своем исходнике написал именно так, т.е. сблокировал весь цикл. Хоть он и Томас и тем более Дивайн, я то сразу подумал, что это не лучшая идея.

[Ochkarik]

не, тогда стоп.
а зачем вам вообще блокировка спинлоками нужна?

[resource]

Потому, что доступ к списку Globals.AdapterList происходит через блокировку Globals.GlobalLock.

Вот такой вариант. Ввел переменную ListChange, которую изменяет MiniportInitialize при добавлении элемента в список

Код:

	Globals.ListChange = TRUE;
	NdisInterlockedInsertTailList(
				&Globals.AdapterList,
				&pAdapt->List,
				&Globals.GlobalLock);

  и MiniportHalt при удалении элелемента

Код:

	Globals.ListChange = TRUE;
	ND_INTERLOCKED_RemoveEntryList(&(pAdapt)->List, &(Globals).GlobalLock);

ND_INTERLOCKED_RemoveEntryList - это макрос - сблокированная RemoveEntryList

После переноса блокировки в цикл, моя функция выглядит так (привожу целеком)

Код:

NTSTATUS
NdEnumerateBindings(
	IN PDEVICE_OBJECT	DeviceObject,
	IN PIRP				Irp
	)
{
	NTSTATUS			Status = STATUS_SUCCESS;
	PIO_STACK_LOCATION	pStack;
	PUCHAR				pBuffer;
	ULONG				OutBufLen;
	PADAPT				pAdapt;
	ULONG_PTR			BytesWritten = 0;
	PLIST_ENTRY			pListHead, pListEntry;
	KIRQL				PrevIrql;

	UNREFERENCED_PARAMETER(DeviceObject);

	DBGPRINT(("---> NdEnumerateBindings\n"));

	pStack = IoGetCurrentIrpStackLocation(Irp);

	OutBufLen = pStack->Parameters.DeviceIoControl.OutputBufferLength;	
	pBuffer = Irp->AssociatedIrp.SystemBuffer;	

	pListHead = &Globals.AdapterList;
	pListEntry = pListHead;

	NdisAcquireSpinLock(&Globals.GlobalLock);

	Globals.ListChange = FALSE;

	while (pListEntry->Flink != pListHead)
	{	
		pListEntry = pListEntry->Flink;
		pAdapt = (PADAPT)pListEntry;

		BytesWritten += pAdapt->DeviceName.Length + sizeof(UNICODE_NULL);
		BytesWritten += pAdapt->LowerDeviceName.Length + sizeof(UNICODE_NULL);
	}

	NdisReleaseSpinLock(&Globals.GlobalLock);

	if (OutBufLen == 0)
	{
		Status = STATUS_BUFFER_OVERFLOW;
		goto LblEnd;
	}
	else if (OutBufLen < BytesWritten)
	{
		Status = STATUS_BUFFER_TOO_SMALL;
		goto LblEnd;
	}

	pListEntry = pListHead;

	NDIS_RAISE_IRQL_TO_DISPATCH(&PrevIrql);

	NdisDprAcquireSpinLock(&Globals.GlobalLock);

	if (Globals.ListChange)
	{
		NdisDprReleaseSpinLock(&Globals.GlobalLock);
		NDIS_LOWER_IRQL(PrevIrql, DISPATCH_LEVEL);
		Status = STATUS_UNSUCCESSFUL;
		goto LblEnd;
	}

	while (pListEntry->Flink != pListHead)
	{	
		pListEntry = pListEntry->Flink;
		pAdapt = (PADAPT)pListEntry;

		RtlCopyMemory(pBuffer,
						pAdapt->DeviceName.Buffer,
						pAdapt->DeviceName.Length);

		pBuffer +=  pAdapt->DeviceName.Length;
		*((PWCHAR )pBuffer) = UNICODE_NULL;
		pBuffer += sizeof(UNICODE_NULL);

		RtlCopyMemory(pBuffer,
						pAdapt->LowerDeviceName.Buffer,
						pAdapt->LowerDeviceName.Length);

		pBuffer +=  pAdapt->LowerDeviceName.Length;
		*((PWCHAR )pBuffer) = UNICODE_NULL;
		pBuffer += sizeof(UNICODE_NULL);

		NdisDprReleaseSpinLock(&Globals.GlobalLock);

		NdisDprAcquireSpinLock(&Globals.GlobalLock);

		if (Globals.ListChange)
		{
			Status = STATUS_UNSUCCESSFUL;
			break;
		}
	}

	NdisDprReleaseSpinLock(&Globals.GlobalLock);
	NDIS_LOWER_IRQL(PrevIrql, DISPATCH_LEVEL);

LblEnd:

	Irp->IoStatus.Information = BytesWritten;

	DBGPRINT(("<--- NdEnumerateBindings [BytesWrtn: %u] [Status: 0x%x]\n", BytesWritten, Status));

	return Status;

То, что я вижу "замыленным" глазом - вроде бы всё нормально. Единственное что меня смущает это освобождение и тут же сразу захват блокировки. Обычно делается как-то так

Код:

	NdisReleaseSpinLock(&Globals.GlobalLock);
	
	NdisMSleep(1);

	NdisAcquireSpinLock(&Globals.GlobalLock);

Только в моем случае это был бы NdisStallExecution. Но надо ли вообще это делать в данном случае? Мне-то вроде бы ждать нечего.

[Ochkarik]

NdisMSleep(1);
нужен для того чтобы другие процессы имели большую вероятность захватить спинлок и не ожидать его слишком долго. это ТОЛЬКО если вы работаете на PASSIVE_LEVEL или APC. на DISPATCH это делать бесполезно, более того - вредно.
то ее можно заменить на Sleep(0), которая в общем случае просто сбрасывает текущий квант времени процесса, в следствии чего происходит принудительное переключение процессов. у руссиновича это очень красиво объяснено.

то есть вам нужно заблокировать удаление элемента списка, в моменты когда кто то его читает?

кстати все равно не получается... условие (pListEntry->Flink != pListHead) не заблокированным остается.
может быть на время работы с элементом - временно его извлекать из списка а потом помещать обратно? - тоже не получается... размер списка будет неизвестен. или как-то можно подумать?

[resource]

Как это (pListEntry->Flink != pListHead) не заблокировано. Перед входом в цикл блокирует NdisDprAcquireSpinLock(&Globals.GlobalLock), жалко строки не нумерованы. Потом разблокируется и снова блокируется парой

Код:

	NdisDprReleaseSpinLock(&Globals.GlobalLock);

	NdisDprAcquireSpinLock(&Globals.GlobalLock);

А потом проверка (уже в блоке) Globals.ListChange. Если переменная впорядке, продолжаем.

Я, честно говоря, не знаю какими Sleep'ами можно пользоваться на DISPATCH_LEVEL. Знаю что KeStallExecutionProcessor можно. Но она, как написано, вообще "блокирует" проц, т.е. никакая другая задача на нём явно не будет выполняться (ну разве что выше DISPATCH).
Но, думаю вы правы, и в моем случае никакое ожидание не требуется.

[Ochkarik]

pListEntry->Flink != pListHead
не заблокировано от добавления элементов.. хотя может и ничего страшного... погорячился)

PS "goto LblEnd;" не стоит - читабельность кода немного снижается... здесь лучше по else{} обернуть продолжение)

[Ochkarik]

кстати посмотрите заодно, интересная ссылка
http://alter.org.ua/ru/docs/nt_kernel/rwlock/

[resource]

pListEntry->Flink != pListHead
не заблокировано от добавления элементов..

Да гдеж не заблокировано. Я приводил уже, что добавление элементов может происходить только в двух местах: MiniportInitialize и MiniportHalt. Там они тоже добаляются через Globals.GlobalLock (код приводил). Элемент действительно может быть добавлен между NdisDprReleaseSpinLock(&Globals.GlobalLock) и NdisDprAcquireSpinLock(&Globals.GlobalLock). Но сразу после NdisDprAcquireSpinLock происходит проверка Globals.ListChange, и есла она FALSE, то элемент не был добавлен или удален. И далее, не снимая блокировки, уже pListEntry->Flink != pListHead. Всё сблокировано получается.

За ссылку спасибо.

А вот насчет "goto LblEnd" и читабельности кода я с вами абсолютно не согласен. Это конечно хорошее правило - не пользоваться goto. Но если вы слепо исполняете это назидание, не смотря на ситуацию, то думаю, посмотрев на исходники WDK (DDK), вы будете шокированы. goto используется очень много где, начиная от toaster'а и еще в куче других (уже более серьезных). Да вы сами посмотрите на код и еще раз на это "правило". Если метка одна на всю функцию, то я не понимаю как это может снизить читабельность кода. Структура функции от этого не становится витееватой, переходов "назад" нет, да и более того, даже "вперед" переход только один (на одно место). С другой стороны, вы посмотрите на код. goto вызывается 3 раза. Кому нужна такая вложенность на ровном месте. К тому же конструкция

Код:

        if (OutBufLen == 0)
	{
		Status = STATUS_BUFFER_OVERFLOW;
		goto LblEnd;
	}
	else if (OutBufLen < BytesWritten)
	{
		Status = STATUS_BUFFER_TOO_SMALL;
		goto LblEnd;
	}

   вообще, приобретет сатанинский облик если сделать ее без goto. C goto всегда надо действовать по ситуации ИМХО. Главное правило которым пользуюсь я, сам для себя, это чтоб не было goto "назад", потому что это уже реально рвет визуальную структуру. "Вперед" лучше бы конечно тоже поменьше, но тут уж, опять же "по ситуации". Если метка только одна на всю функцию это вообще нормально.

[Алексей++]

Код:

for(;;)
{
	...
	if (OutBufLen == 0)
	{
		Status = STATUS_BUFFER_OVERFLOW;
		break;
	}

	if (OutBufLen < BytesWritten)
	{
		Status = STATUS_BUFFER_TOO_SMALL;
		break;
	}
	...
	break;
}
//LblEnd:

[resource]

Ну тут у каждого своё "хорошо" и "плохо". Мне такое решение красивым не кажется. Кому-то кажется goto плохим решением, а мне вложенность кажется не всегда хорошим делом. Хотя признаю, конечно, что несколько преувеличил  
Кроме того, такой подход будет работать до тех пор, пока мне не понадобиться перейти на LblEnd (касаемо этого конкретного примера) из while, который будет вложен в for( ;; ). Я конечно пользуюсь вещами типа do { } while (FALSE); но иногда и goto делаю. в общем, как уже сказал, по ситуации.

[Алексей++]

пооффтоплю немножко

я не агитирую совсем не пользоваться goto, но:

если логика такова, что выйти по метке потребовалось из самых неожиданных мест, то выхода два:
(гы, вспомнилось: "даже если вас съели, у вас есть два выхода")

1)
 если основное время крутимся в этом сложном цикле, относительно редко проходя через точку try (иначе теряем в производительности), то:

Код:

try
{
     //хоть где -
     throw 0;
}
catch(...)
{
}
//LblEnd:

2) выделить в отдельную функцию типа bool, передав все нужные параметры по ссылке (тогда даже менять имена в блоке не придётся) , а выход сделать return false.

[resource]

Вы уж извините, но try-catch-throw мне кажется совершенно диким вариантом для таких целей. Это конечно только моё мнение. А так может это на самом деле очень красиво и изящно

[Ochkarik]

Алексей1153++,  ну... эксепшены эт действительно перебор....
это даже как-то неприлично предлагать)
что до точки try и производительности- ребята рассказывали байку про отличный метод, помещать Try в начале main())))))

resource,
дело вкуса... я подумал я же размещаю return-ы посреди функции... по читаемости в принципе то же... не очень) а куда деваться)

[resource]

return-ы посреди функции это вообще нормально ИМХО. По крайней мере если функция так построена, что всё нормально получается, то почему бы нет. Конечно более стройным считается такой код, в котором return происходит в конце функции. Я собственно так и пытаюсь делать (как и в приведенном коде). Но если бы было удобнее сделать return посреди функции, не колебался бы ни секунды 

[Алексей++]

Вы уж извините, но try-catch-throw мне кажется совершенно диким вариантом для таких целей. Это конечно только моё мнение. А так может это на самом деле очень красиво и изящно

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

[Ochkarik]

Алексей1153++, обычно 50-90% драйвера это проверка статуса после вызова каждой функции, и корректный откат в случае ошибки.
то-есть логических точек выхода из функции может быть довольно много, и везде по разному.
в среднем в одной не очень крупной функции(на страницу кода примерно) у меня может быть порядка 5 различных нештатных ситуаций, которые надо по разному обработать.(я подчеркиваю - В СРЕДНЕМ!).
очень неудобно тянуть каждую ситуацию до конца функции, загромождает сильно.
к примеру, в приложении проще - не выделилась память по new() - ну  и хрен с ней, работать дальше бессмысленно, гори все синим пламенем. а в драйвере даже если ты 10 байт выделяешь - просто ОБЯЗАН проверить статус и вернуть все как было, чтобы операционка не грохнулась) иначе появляются байки про кривую windows))
внутри каждой функции внутренние статусы вводить - тоже как-то не очень.
разбивать на более мелкие функции - вроде тоже не всегда красиво... потом по коду прыгать. да и.... кстати было выражение что мол каждую функцию надо вызывать так, как будто ее написал идиот) даже если написал ее сам))) у меня такой подход) по возможности.

[Алексей++]

Ochkarik,

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

а я что - противоречу что ли ?

2) выделить в отдельную функцию типа bool, передав все нужные параметры по ссылке (тогда даже менять имена в блоке не придётся) , а выход сделать return false.