Re: [win-pv-devel] [PATCH 4/4] Use a cache for header buffers, to fix a 'function uses too much stack' warning

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Owen Smith [mailto:owen.smith@xxxxxxxxxx]
> Sent: 12 November 2014 16:39
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Paul Durrant; Owen Smith
> Subject: [PATCH 4/4] Use a cache for header buffers, to fix a 'function uses
> too much stack' warning
> 
> Tweak GetPacketHeaders interface call to pass less parameters.
> 
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> ---
>  include/vif_interface.h  |  16 +++---
>  src/xennet/transmitter.c | 142
> ++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 136 insertions(+), 22 deletions(-)
> 
> diff --git a/include/vif_interface.h b/include/vif_interface.h
> index 83e3846..5662293 100644
> --- a/include/vif_interface.h
> +++ b/include/vif_interface.h
> @@ -528,16 +528,18 @@ typedef NTSTATUS
>      \brief Get a copy of the packet headers and each the offset of each
> 
>      \param Interface The interface header
> +    \param Packet The packet
> +    \param HeaderBuffer Buffer to receive a copy of the headers
> +    \param HeaderLength Length in bytes of Buffer
> +    \param Info Location to receive offsets of headers into Buffer
>  */
>  typedef NTSTATUS
>  (*XENVIF_VIF_TRANSMITTER_GET_PACKET_HEADERS)(
> -    IN  PINTERFACE          Interface,
> -    IN  PMDL                Mdl,
> -    IN  ULONG               Offset,
> -    IN  ULONG               Length,
> -    OUT PVOID               HeaderBuffer,
> -    IN  ULONG               HeaderLength,
> -    OUT PXENVIF_PACKET_INFO Info
> +    IN  PINTERFACE                      Interface,
> +    IN  PXENVIF_TRANSMITTER_PACKET_V2   Packet,
> +    OUT PVOID                           HeaderBuffer,
> +    IN  ULONG                           HeaderLength,
> +    OUT PXENVIF_PACKET_INFO             Info
>      );
> 
>  /*! \typedef XENVIF_VIF_TRANSMITTER_QUERY_OFFLOAD_OPTIONS
> diff --git a/src/xennet/transmitter.c b/src/xennet/transmitter.c
> index e408364..4738eed 100644
> --- a/src/xennet/transmitter.c
> +++ b/src/xennet/transmitter.c
> @@ -43,10 +43,13 @@ struct _XENNET_TRANSMITTER {
>      XENVIF_VIF_OFFLOAD_OPTIONS  OffloadOptions;
> 
>      PXENBUS_CACHE_INTERFACE     CacheInterface;
> -    PXENBUS_CACHE               Cache;
> +    PXENBUS_CACHE               PacketCache;
> +    PXENBUS_CACHE               BufferCache;
> +    KSPIN_LOCK                  Lock;
>  };
> 
>  #define TRANSMITTER_POOL_TAG    'teNX'
> +#define BUFFER_SIZE             PAGE_SIZE
> 
>  static NTSTATUS
>  __PacketCtor(
> @@ -74,7 +77,8 @@ __PacketAcquire(
>      IN  PVOID           Argument
>      )
>  {
> -    UNREFERENCED_PARAMETER(Argument);
> +    PXENNET_TRANSMITTER Transmitter = Argument;
> +    KeAcquireSpinLockAtDpcLevel(&Transmitter->Lock);
>  }
> 
>  static VOID
> @@ -82,7 +86,47 @@ __PacketRelease(
>      IN  PVOID           Argument
>      )
>  {
> +    PXENNET_TRANSMITTER Transmitter = Argument;
> +    KeReleaseSpinLockFromDpcLevel(&Transmitter->Lock);
> +}

Ah, I see you implemented the locking here. This needs to be moved to patch #2.

  Paul

> +
> +static NTSTATUS
> +__BufferCtor(
> +    IN  PVOID           Argument,
> +    IN  PVOID           Object
> +    )
> +{
> +    UNREFERENCED_PARAMETER(Argument);
> +    RtlZeroMemory(Object, BUFFER_SIZE);
> +    return STATUS_SUCCESS;
> +}
> +
> +static VOID
> +__BufferDtor(
> +    IN  PVOID           Argument,
> +    IN  PVOID           Object
> +    )
> +{
>      UNREFERENCED_PARAMETER(Argument);
> +    UNREFERENCED_PARAMETER(Object);
> +}
> +
> +static VOID
> +__BufferAcquire(
> +    IN  PVOID           Argument
> +    )
> +{
> +    PXENNET_TRANSMITTER Transmitter = Argument;
> +    KeAcquireSpinLockAtDpcLevel(&Transmitter->Lock);
> +}
> +
> +static VOID
> +__BufferRelease(
> +    IN  PVOID           Argument
> +    )
> +{
> +    PXENNET_TRANSMITTER Transmitter = Argument;
> +    KeReleaseSpinLockFromDpcLevel(&Transmitter->Lock);
>  }
> 
>  static FORCEINLINE PXENVIF_TRANSMITTER_PACKET_V2
> @@ -92,11 +136,11 @@ __TransmitterGetPacket(
>  {
>      ASSERT3U(XENVIF_VIF_VERSION(Transmitter->VifInterface), ==, 2);
>      ASSERT(Transmitter->CacheInterface != NULL);
> -    ASSERT(Transmitter->Cache != NULL);
> +    ASSERT(Transmitter->PacketCache != NULL);
> 
>      return XENBUS_CACHE(Get,
>                          Transmitter->CacheInterface,
> -                        Transmitter->Cache,
> +                        Transmitter->PacketCache,
>                          FALSE);
>  }
> 
> @@ -108,15 +152,47 @@ __TransmitterPutPacket(
>  {
>      ASSERT3U(XENVIF_VIF_VERSION(Transmitter->VifInterface), ==, 2);
>      ASSERT(Transmitter->CacheInterface != NULL);
> -    ASSERT(Transmitter->Cache != NULL);
> +    ASSERT(Transmitter->PacketCache != NULL);
> 
>      XENBUS_CACHE(Put,
>                   Transmitter->CacheInterface,
> -                 Transmitter->Cache,
> +                 Transmitter->PacketCache,
>                   Packet,
>                   FALSE);
>  }
> 
> +static FORCEINLINE PVOID
> +__TransmitterGetBuffer(
> +    IN  PXENNET_TRANSMITTER     Transmitter
> +    )
> +{
> +    ASSERT3U(XENVIF_VIF_VERSION(Transmitter->VifInterface), ==, 2);
> +    ASSERT(Transmitter->CacheInterface != NULL);
> +    ASSERT(Transmitter->BufferCache != NULL);
> +
> +    return XENBUS_CACHE(Get,
> +                        Transmitter->CacheInterface,
> +                        Transmitter->BufferCache,
> +                        FALSE);
> +}
> +
> +static FORCEINLINE VOID
> +__TransmitterPutBuffer(
> +    IN  PXENNET_TRANSMITTER     Transmitter,
> +    IN  PVOID                   Buffer
> +    )
> +{
> +    ASSERT3U(XENVIF_VIF_VERSION(Transmitter->VifInterface), ==, 2);
> +    ASSERT(Transmitter->CacheInterface != NULL);
> +    ASSERT(Transmitter->BufferCache != NULL);
> +
> +    XENBUS_CACHE(Put,
> +                 Transmitter->CacheInterface,
> +                 Transmitter->BufferCache,
> +                 Buffer,
> +                 FALSE);
> +}
> +
>  typedef struct _NET_BUFFER_LIST_RESERVED {
>      LONG    Reference;
>  } NET_BUFFER_LIST_RESERVED, *PNET_BUFFER_LIST_RESERVED;
> @@ -289,24 +365,27 @@ __TransmitterCalculateHash(
>      IN  PXENVIF_TRANSMITTER_PACKET_V2   Packet
>      )
>  {
> -    UCHAR               Buffer[1024];
> +    PUCHAR              Buffer;
>      UCHAR               HashOver[40]; // sizeof(IPV6_ADDRESS)*2 +
> sizeof(USHORT)*2
>      PUCHAR              Ptr;
>      XENVIF_PACKET_INFO  Info;
>      NTSTATUS            status;
> 
> +    status = STATUS_NO_MEMORY;
> +    Buffer = __TransmitterGetBuffer(Transmitter);
> +    if (Buffer == NULL)
> +        goto fail1;
> +
>      RtlZeroMemory(&Info, sizeof(XENVIF_PACKET_INFO));
> 
>      status = XENVIF_VIF(TransmitterGetPacketHeaders,
>                          Transmitter->VifInterface,
> -                        Packet->Mdl,
> -                        Packet->Offset,
> -                        Packet->Length,
> +                        Packet,
>                          Buffer,
> -                        sizeof(Buffer),
> +                        BUFFER_SIZE,
>                          &Info);
>      if (!NT_SUCCESS(status))
> -        goto fail1;
> +        goto fail2;
> 
>      Ptr = HashOver;
>      if (Info.IpHeader.Length) {
> @@ -345,6 +424,9 @@ __TransmitterCalculateHash(
>          *(PUSHORT)Ptr = UdpHeader->DestinationPort;
>          Ptr += sizeof(USHORT);
>      }
> +
> +    __TransmitterPutBuffer(Transmitter, Buffer);
> +
>      if (Ptr - HashOver == 0)
>          goto done;
> 
> @@ -353,6 +435,11 @@ __TransmitterCalculateHash(
>  done:
>      return 0;
> 
> +fail2:
> +    Error("fail2\n");
> +
> +    __TransmitterPutBuffer(Transmitter, Buffer);
> +
>  fail1:
>      Error("fail1 (%08x)\n", status);
> 
> @@ -619,6 +706,7 @@ TransmitterInitialize(
>      if (XENVIF_VIF_VERSION((*Transmitter)->VifInterface) == 1)
>          goto done;
> 
> +    KeInitializeSpinLock(&(*Transmitter)->Lock);
>      (*Transmitter)->CacheInterface = AdapterGetCacheInterface(Adapter);
>      status = XENBUS_CACHE(Acquire, (*Transmitter)->CacheInterface);
>      if (!NT_SUCCESS(status))
> @@ -634,19 +722,38 @@ TransmitterInitialize(
>                            __PacketAcquire,
>                            __PacketRelease,
>                            *Transmitter,
> -                          &(*Transmitter)->Cache);
> +                          &(*Transmitter)->PacketCache);
>      if (!NT_SUCCESS(status))
>          goto fail4;
> 
> +    status = XENBUS_CACHE(Create,
> +                          (*Transmitter)->CacheInterface,
> +                          "buffers",
> +                          BUFFER_SIZE,
> +                          4,
> +                          __BufferCtor,
> +                          __BufferDtor,
> +                          __BufferAcquire,
> +                          __BufferRelease,
> +                          *Transmitter,
> +                          &(*Transmitter)->BufferCache);
> +    if (!NT_SUCCESS(status))
> +        goto fail5;
> +
>  done:
>      return STATUS_SUCCESS;
> 
> +fail5:
> +    Error("fail5\n");
> +    XENBUS_CACHE(Destroy, (*Transmitter)->CacheInterface,
> (*Transmitter)->PacketCache);
> +    (*Transmitter)->PacketCache = NULL;
>  fail4:
>      Error("fail4\n");
>      XENBUS_CACHE(Release, (*Transmitter)->CacheInterface);
>      (*Transmitter)->CacheInterface = NULL;
>  fail3:
>      Error("fail3\n");
> +    RtlZeroMemory(&(*Transmitter)->Lock, sizeof(KSPIN_LOCK));
>      XENVIF_VIF(Release, (*Transmitter)->VifInterface);
>      (*Transmitter)->VifInterface = NULL;
>  fail2:
> @@ -666,12 +773,17 @@ TransmitterTeardown(
>      if (XENVIF_VIF_VERSION(Transmitter->VifInterface) == 1)
>          goto done;
> 
> -    XENBUS_CACHE(Destroy, Transmitter->CacheInterface, Transmitter-
> >Cache);
> -    Transmitter->Cache = NULL;
> +    XENBUS_CACHE(Destroy, Transmitter->CacheInterface, Transmitter-
> >BufferCache);
> +    Transmitter->BufferCache = NULL;
> +
> +    XENBUS_CACHE(Destroy, Transmitter->CacheInterface, Transmitter-
> >PacketCache);
> +    Transmitter->PacketCache = NULL;
> 
>      XENBUS_CACHE(Release, Transmitter->CacheInterface);
>      Transmitter->CacheInterface = NULL;
> 
> +    RtlZeroMemory(&Transmitter->Lock, sizeof(KSPIN_LOCK));
> +
>  done:
>      XENVIF_VIF(Release, Transmitter->VifInterface);
>      Transmitter->VifInterface = NULL;
> --
> 1.9.4.msysgit.1


_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel