[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Windows PV drivers fail to set up RSS when vCPU index >= 8
- To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
- From: "Durrant, Paul" <xadimgnik@xxxxxxxxx>
- Date: Wed, 23 Mar 2022 17:59:11 +0100
- Delivery-date: Wed, 23 Mar 2022 16:59:18 +0000
- List-id: Developer list for the Windows PV Drivers subproject <win-pv-devel.lists.xenproject.org>
On 23/03/2022 17:24, Durrant, Paul wrote:
On 23/03/2022 11:33, Martin Harvey wrote:
The driver only supports at most 8 queues, however Windows
can decide to assign vCPU numbers starting from a non-zero
offset. E.g. vCPU 8,9,10,11 could get assigned to a device
if you have more than one NIC. The total number of vCPUs
used by a single device is still less than 8, but the vCPU
indexes themselves can be greater than 8. The code
previously incorrectly assumed that individual vCPU
indexes cannot exceed 8, however a 1:1 mapping between
vCPU indexes and queues seems to only exist when using
a single NIC.
Signed-off-by: Martin Harvey <martin.harvey@xxxxxxxxxx>
Acked-by: Paul Durrant <paul@xxxxxxx>
... with the following observations...
---
src/xenvif/receiver.c | 161 ++++++++++++++++++++++++++++++++++++------
1 file changed, 141 insertions(+), 20 deletions(-)
diff --git a/src/xenvif/receiver.c b/src/xenvif/receiver.c
index 10ac6f5..fae9b31 100644
--- a/src/xenvif/receiver.c
+++ b/src/xenvif/receiver.c
@@ -106,6 +106,7 @@ typedef struct _XENVIF_RECEIVER_RING {
PLIST_ENTRY PacketQueue;
KDPC QueueDpc;
ULONG QueueDpcs;
+ PROCESSOR_NUMBER TargetProcessor;
LIST_ENTRY PacketComplete;
XENVIF_RECEIVER_HASH Hash;
} XENVIF_RECEIVER_RING, *PXENVIF_RECEIVER_RING;
@@ -2498,6 +2499,9 @@ __ReceiverRingInitialize(
KeInitializeThreadedDpc(&(*Ring)->QueueDpc,
ReceiverRingQueueDpc, *Ring);
+ status = KeGetProcessorNumberFromIndex((*Ring)->Index,
&(*Ring)->TargetProcessor);
+ ASSERT(NT_SUCCESS(status));
+
return STATUS_SUCCESS;
fail7:
@@ -2550,6 +2554,41 @@ fail1:
return status;
}
+static FORCEINLINE NTSTATUS
+__ReceiverRingSetAffinity(
+ IN PXENVIF_RECEIVER_RING Ring,
+ IN PPROCESSOR_NUMBER Processor
+ )
+{
+ PXENVIF_RECEIVER Receiver;
+ PXENVIF_FRONTEND Frontend;
+ NTSTATUS status;
misalignment here... I can fix
+
+ status = STATUS_INVALID_PARAMETER;
+ if ((Ring == NULL) || (Processor == NULL))
extra brackets not needed... I will fix
Actually this check can go away. This function is static with a single
caller where both of these arguments can't be NULL.
+ goto fail1;
+
+ Receiver = Ring->Receiver;
+ Frontend = Receiver->Frontend;
+
+ __ReceiverRingAcquireLock(Ring);
+
+ Ring->TargetProcessor = *Processor;
+
+ /* Don't rebind event channel at this point. */
+ KeSetTargetProcessorDpcEx(&Ring->PollDpc, &Ring->TargetProcessor);
+ KeSetTargetProcessorDpcEx(&Ring->QueueDpc, &Ring->TargetProcessor);
+
+ __ReceiverRingReleaseLock(Ring);
+
+ return STATUS_SUCCESS;
+
+fail1:
+ Error("fail1 (%08x)\n", status);
+
+ return status;
+}
+
static FORCEINLINE NTSTATUS
__ReceiverRingConnect(
IN PXENVIF_RECEIVER_RING Ring
@@ -2560,7 +2599,6 @@ __ReceiverRingConnect(
PFN_NUMBER Pfn;
CHAR Name[MAXNAMELEN];
ULONG Index;
- PROCESSOR_NUMBER ProcNumber;
NTSTATUS status;
Receiver = Ring->Receiver;
@@ -2637,16 +2675,17 @@ __ReceiverRingConnect(
if (Ring->Channel == NULL)
goto fail6;
- status = KeGetProcessorNumberFromIndex(Ring->Index, &ProcNumber);
- ASSERT(NT_SUCCESS(status));
-
- KeSetTargetProcessorDpcEx(&Ring->PollDpc, &ProcNumber);
+ status = XENBUS_EVTCHN(Bind,
+ &Receiver->EvtchnInterface,
+ Ring->Channel,
+ Ring->TargetProcessor.Group,
+ Ring->TargetProcessor.Number);
+ if (!NT_SUCCESS(status))
+ Warning("Could not set initial receiver ring affinity: 0x%x
for ring %u\n", status, Ring->Index);
+ /* Ring affinity not yet changed from initial CPU number, so just
warn. */
comment is not well placed... I'll fix
- (VOID) XENBUS_EVTCHN(Bind,
- &Receiver->EvtchnInterface,
- Ring->Channel,
- ProcNumber.Group,
- ProcNumber.Number);
+ KeSetTargetProcessorDpcEx(&Ring->PollDpc, &Ring->TargetProcessor);
+ KeSetTargetProcessorDpcEx(&Ring->QueueDpc, &Ring->TargetProcessor);
(VOID) XENBUS_EVTCHN(Unmask,
&Receiver->EvtchnInterface,
@@ -2665,11 +2704,6 @@ __ReceiverRingConnect(
if (!NT_SUCCESS(status))
goto fail7;
- status = KeGetProcessorNumberFromIndex(Ring->Index, &ProcNumber);
- ASSERT(NT_SUCCESS(status));
-
- KeSetTargetProcessorDpcEx(&Ring->QueueDpc, &ProcNumber);
-
return STATUS_SUCCESS;
fail7:
@@ -3917,6 +3951,56 @@ fail1:
return status;
}
+static NTSTATUS
+__ReceiverSetQueueAffinities(
still inconsistent so I'll make this FORCEINLINE
+ IN PXENVIF_RECEIVER Receiver,
+ IN PPROCESSOR_NUMBER QueueAffinities,
+ IN ULONG Count
+ )
+{
+ PXENVIF_FRONTEND Frontend;
+ ULONG Index;
+ NTSTATUS status;
+ KIRQL Irql;
+
+ Frontend = Receiver->Frontend;
+
+ status = STATUS_INVALID_PARAMETER;
+
+ if (QueueAffinities == NULL)
+ goto fail1;
+
+ if (Count > FrontendGetNumQueues(Frontend))
+ goto fail2;
+
+ KeRaiseIrql(DISPATCH_LEVEL, &Irql);
+
+ for (Index = 0; Index < Count; Index++) {
+ PXENVIF_RECEIVER_RING Ring = Receiver->Ring[Index];
+
+ status = __ReceiverRingSetAffinity(Ring,
&QueueAffinities[Index]);
+ if (!NT_SUCCESS(status))
+ goto fail3;
+ }
+
+ KeLowerIrql(Irql);
+
+ return STATUS_SUCCESS;
+
+fail3:
+ KeLowerIrql(Irql);
+
+ Error("fail3\n");
the error message should go immediately after the label... I will fix
+
+fail2:
+ Error("fail2\n");
+
+fail1:
+ Error("fail1 (%08x)\n", status);
+
+ return status;
+}
+
NTSTATUS
ReceiverUpdateHashMapping(
IN PXENVIF_RECEIVER Receiver,
@@ -3926,7 +4010,10 @@ ReceiverUpdateHashMapping(
{
PXENVIF_FRONTEND Frontend;
PULONG QueueMapping;
+ PPROCESSOR_NUMBER QueueAffinities;
ULONG NumQueues;
+ ULONG QueuesDetermined;
+ ULONG QueueIndex;
ULONG Index;
NTSTATUS status;
@@ -3939,26 +4026,60 @@ ReceiverUpdateHashMapping(
goto fail1;
NumQueues = FrontendGetNumQueues(Frontend);
+ QueuesDetermined = 0;
+
+ QueueAffinities = __ReceiverAllocate(sizeof(PROCESSOR_NUMBER) *
NumQueues);
+ if (QueueAffinities == NULL)
+ goto fail2;
status = STATUS_INVALID_PARAMETER;
+ /* N^Squared-ish, but performed infrequently */
for (Index = 0; Index < Size; Index++) {
- QueueMapping[Index] =
KeGetProcessorIndexFromNumber(&ProcessorMapping[Index]);
-
- if (QueueMapping[Index] >= NumQueues)
- goto fail2;
+ BOOLEAN MapEntryDone = FALSE;
blank line needed here... I will fix
+ /* Existing queue meets affinity requirement for the mapping
at this index? */
I'll fix this to be a proper sentence
+ for (QueueIndex = 0; QueueIndex < QueuesDetermined;
QueueIndex++) {
+ if ((QueueAffinities[QueueIndex].Group ==
ProcessorMapping[Index].Group) &&
+ (QueueAffinities[QueueIndex].Number ==
ProcessorMapping[Index].Number)) {
+ QueueMapping[Index] = QueueIndex;
+ MapEntryDone = TRUE;
+ }
+ }
+ if (!MapEntryDone) {
+ /* New queue "allocation", with new affinity, if possible */
+ if (QueuesDetermined >= NumQueues)
+ goto fail3;
+
+ QueueIndex = QueuesDetermined;
+ QueueAffinities[QueueIndex] = ProcessorMapping[Index];
+ QueueMapping[Index] = QueueIndex;
+ QueuesDetermined++;
+ }
}
status = FrontendSetHashMapping(Frontend, QueueMapping, Size);
if (!NT_SUCCESS(status))
- goto fail3;
+ goto fail4;
+
+ status = __ReceiverSetQueueAffinities(Receiver, QueueAffinities,
QueuesDetermined);
+ if (!NT_SUCCESS(status))
+ goto fail5;
__ReceiverFree(QueueMapping);
+ __ReceiverFree(QueueAffinities);
return STATUS_SUCCESS;
+fail5:
+ Error("fail5\n");
+
+fail4:
+ Error("fail4\n");
+
fail3:
Error("fail3\n");
+ __ReceiverFree(QueueAffinities);
+
fail2:
Error("fail2\n");
|
