public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Fu, Siyuan" <siyuan.fu@intel.com>
To: "Wu, Jiaxin" <jiaxin.wu@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Wang, Fan" <fan.wang@intel.com>, "Ye, Ting" <ting.ye@intel.com>
Subject: Re: [Patch] MdeModulePkg/Mtftp4Dxe: Separate the timer ticking to calculate the packet live time.
Date: Fri, 2 Mar 2018 06:45:52 +0000	[thread overview]
Message-ID: <B1FF2E9001CE9041BD10B825821D5BC58B449E02@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <20180302064322.3600-1-jiaxin.wu@intel.com>

Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>


> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Friday, March 2, 2018 2:43 PM
> To: edk2-devel@lists.01.org
> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wang, Fan <fan.wang@intel.com>; Ye,
> Ting <ting.ye@intel.com>
> Subject: [Patch] MdeModulePkg/Mtftp4Dxe: Separate the timer ticking to
> calculate the packet live time.
> 
> From: Fu Siyuan <siyuan.fu@intel.com>
> 
> TPL deadlock issue was enrolled by the commit of 39b0867d. To resolve the
> issue,
> this patch separated the timer ticking for all the MTFTP clients to
> calculate the
> packet live time in TPL_NOTIFY level.
> 
> Cc: Wang Fan <fan.wang@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Ye Ting <ting.ye@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Fu Siyuan <siyuan.fu@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  .../Universal/Network/Mtftp4Dxe/Mtftp4Driver.c     | 34 ++++++++++---
>  .../Universal/Network/Mtftp4Dxe/Mtftp4Impl.c       |  5 +-
>  .../Universal/Network/Mtftp4Dxe/Mtftp4Impl.h       |  6 ++-
>  .../Universal/Network/Mtftp4Dxe/Mtftp4Support.c    | 57
> +++++++++++++++++-----
>  .../Universal/Network/Mtftp4Dxe/Mtftp4Support.h    | 16 +++++-
>  5 files changed, 97 insertions(+), 21 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Driver.c
> b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Driver.c
> index a23d405baa..713cc66dd1 100644
> --- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Driver.c
> +++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Driver.c
> @@ -1,9 +1,9 @@
>  /** @file
>    Implementation of Mtftp drivers.
> 
> -Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
>  http://opensource.org/licenses/bsd-license.php<BR>
> 
> @@ -160,15 +160,16 @@ Mtftp4CreateService (
>    MtftpSb->Signature      = MTFTP4_SERVICE_SIGNATURE;
>    MtftpSb->ServiceBinding = gMtftp4ServiceBindingTemplete;
>    MtftpSb->ChildrenNum    = 0;
>    InitializeListHead (&MtftpSb->Children);
> 
> -  MtftpSb->Timer          = NULL;
> -  MtftpSb->TimerToGetMap  = NULL;
> -  MtftpSb->Controller     = Controller;
> -  MtftpSb->Image          = Image;
> -  MtftpSb->ConnectUdp     = NULL;
> +  MtftpSb->Timer            = NULL;
> +  MtftpSb->TimerNotifyLevel = NULL;
> +  MtftpSb->TimerToGetMap    = NULL;
> +  MtftpSb->Controller       = Controller;
> +  MtftpSb->Image            = Image;
> +  MtftpSb->ConnectUdp       = NULL;
> 
>    //
>    // Create the timer and a udp to be notified when UDP is uninstalled
>    //
>    Status = gBS->CreateEvent (
> @@ -176,12 +177,24 @@ Mtftp4CreateService (
>                    TPL_CALLBACK,
>                    Mtftp4OnTimerTick,
>                    MtftpSb,
>                    &MtftpSb->Timer
>                    );
> +  if (EFI_ERROR (Status)) {
> +    FreePool (MtftpSb);
> +    return Status;
> +  }
> 
> +  Status = gBS->CreateEvent (
> +                  EVT_NOTIFY_SIGNAL | EVT_TIMER,
> +                  TPL_NOTIFY,
> +                  Mtftp4OnTimerTickNotifyLevel,
> +                  MtftpSb,
> +                  &MtftpSb->TimerNotifyLevel
> +                  );
>    if (EFI_ERROR (Status)) {
> +    gBS->CloseEvent (MtftpSb->Timer);
>      FreePool (MtftpSb);
>      return Status;
>    }
> 
>    //
> @@ -194,10 +207,11 @@ Mtftp4CreateService (
>                    NULL,
>                    NULL,
>                    &MtftpSb->TimerToGetMap
>                    );
>    if (EFI_ERROR (Status)) {
> +    gBS->CloseEvent (MtftpSb->TimerNotifyLevel);
>      gBS->CloseEvent (MtftpSb->Timer);
>      FreePool (MtftpSb);
>      return Status;
>    }
> 
> @@ -209,10 +223,11 @@ Mtftp4CreateService (
>                            NULL
>                            );
> 
>    if (MtftpSb->ConnectUdp == NULL) {
>      gBS->CloseEvent (MtftpSb->TimerToGetMap);
> +    gBS->CloseEvent (MtftpSb->TimerNotifyLevel);
>      gBS->CloseEvent (MtftpSb->Timer);
>      FreePool (MtftpSb);
>      return EFI_DEVICE_ERROR;
>    }
> 
> @@ -232,10 +247,11 @@ Mtftp4CleanService (
>    IN MTFTP4_SERVICE     *MtftpSb
>    )
>  {
>    UdpIoFreeIo (MtftpSb->ConnectUdp);
>    gBS->CloseEvent (MtftpSb->TimerToGetMap);
> +  gBS->CloseEvent (MtftpSb->TimerNotifyLevel);
>    gBS->CloseEvent (MtftpSb->Timer);
>  }
> 
> 
>  /**
> @@ -292,10 +308,16 @@ Mtftp4DriverBindingStart (
> 
>    if (EFI_ERROR (Status)) {
>      goto ON_ERROR;
>    }
> 
> +  Status = gBS->SetTimer (MtftpSb->TimerNotifyLevel, TimerPeriodic,
> TICKS_PER_SECOND);
> +
> +  if (EFI_ERROR (Status)) {
> +    goto ON_ERROR;
> +  }
> +
>    //
>    // Install the Mtftp4ServiceBinding Protocol onto Controller
>    //
>    Status = gBS->InstallMultipleProtocolInterfaces (
>                    &Controller,
> diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.c
> b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.c
> index f5f9e6d8f7..d8c48ec8b2 100644
> --- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.c
> +++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.c
> @@ -1080,10 +1080,11 @@ EfiMtftp4Poll (
>    IN EFI_MTFTP4_PROTOCOL    *This
>    )
>  {
>    MTFTP4_PROTOCOL           *Instance;
>    EFI_UDP4_PROTOCOL         *Udp;
> +  EFI_STATUS                Status;
> 
>    if (This == NULL) {
>      return EFI_INVALID_PARAMETER;
>    }
> 
> @@ -1094,11 +1095,13 @@ EfiMtftp4Poll (
>    } else if (Instance->State == MTFTP4_STATE_DESTROY) {
>      return EFI_DEVICE_ERROR;
>    }
> 
>    Udp = Instance->UnicastPort->Protocol.Udp4;
> -  return Udp->Poll (Udp);
> +  Status = Udp->Poll (Udp);
> +  Mtftp4OnTimerTick (NULL, Instance->Service);
> +  return Status;
>  }
> 
>  EFI_MTFTP4_PROTOCOL gMtftp4ProtocolTemplate = {
>    EfiMtftp4GetModeData,
>    EfiMtftp4Configure,
> diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.h
> b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.h
> index 527fd1db10..851b595eee 100644
> --- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.h
> +++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.h
> @@ -7,11 +7,11 @@
>    RFC2090 - TFTP Multicast Option
>    RFC2347 - TFTP Option Extension
>    RFC2348 - TFTP Blocksize Option
>    RFC2349 - TFTP Timeout Interval and Transfer Size Options
> 
> -Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
>  http://opensource.org/licenses/bsd-license.php<BR>
> 
> @@ -70,11 +70,12 @@ struct _MTFTP4_SERVICE {
>    EFI_SERVICE_BINDING_PROTOCOL  ServiceBinding;
> 
>    UINT16                        ChildrenNum;
>    LIST_ENTRY                    Children;
> 
> -  EFI_EVENT                     Timer;  ///< Ticking timer for all the
> MTFTP clients
> +  EFI_EVENT                     Timer;  ///< Ticking timer for all the
> MTFTP clients to handle the packet timeout case.
> +  EFI_EVENT                     TimerNotifyLevel; ///< Ticking timer for
> all the MTFTP clients to calculate the packet live time.
>    EFI_EVENT                     TimerToGetMap;
> 
>    EFI_HANDLE                    Controller;
>    EFI_HANDLE                    Image;
> 
> @@ -133,10 +134,11 @@ struct _MTFTP4_PROTOCOL {
>    //
>    // Timeout and retransmit status
>    //
>    NET_BUF                       *LastPacket;
>    UINT32                        PacketToLive;
> +  BOOLEAN                       HasTimeout;
>    UINT32                        CurRetry;
>    UINT32                        MaxRetry;
>    UINT32                        Timeout;
> 
>    //
> diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.c
> b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.c
> index c625fda020..e4366b6ddb 100644
> --- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.c
> +++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.c
> @@ -1,9 +1,9 @@
>  /** @file
>    Support routines for Mtftp.
> 
> -Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
>  http://opensource.org/licenses/bsd-license.php<BR>
> 
> @@ -566,10 +566,46 @@ Mtftp4Retransmit (
> 
>    return Status;
>  }
> 
> 
> +/**
> +  The timer ticking function in TPL_NOTIFY level for the Mtftp service
> instance.
> +
> +  @param  Event                 The ticking event
> +  @param  Context               The Mtftp service instance
> +
> +**/
> +VOID
> +EFIAPI
> +Mtftp4OnTimerTickNotifyLevel (
> +  IN EFI_EVENT              Event,
> +  IN VOID                   *Context
> +  )
> +{
> +  MTFTP4_SERVICE            *MtftpSb;
> +  LIST_ENTRY                *Entry;
> +  LIST_ENTRY                *Next;
> +  MTFTP4_PROTOCOL           *Instance;
> +
> +  MtftpSb = (MTFTP4_SERVICE *) Context;
> +
> +  //
> +  // Iterate through all the children of the Mtftp service instance. Time
> +  // out the current packet transmit.
> +  //
> +  NET_LIST_FOR_EACH_SAFE (Entry, Next, &MtftpSb->Children) {
> +    Instance = NET_LIST_USER_STRUCT (Entry, MTFTP4_PROTOCOL, Link);
> +    if ((Instance->PacketToLive == 0) || (--Instance->PacketToLive > 0))
> {
> +      Instance->HasTimeout = FALSE;
> +    } else {
> +      Instance->HasTimeout = TRUE;
> +    }
> +  }
> +}
> +
> +
>  /**
>    The timer ticking function for the Mtftp service instance.
> 
>    @param  Event                 The ticking event
>    @param  Context               The Mtftp service instance
> @@ -589,33 +625,32 @@ Mtftp4OnTimerTick (
>    EFI_MTFTP4_TOKEN          *Token;
> 
>    MtftpSb = (MTFTP4_SERVICE *) Context;
> 
>    //
> -  // Iterate through all the children of the Mtftp service instance. Time
> -  // out the packet. If maximum retries reached, clean the session up.
> +  // Iterate through all the children of the Mtftp service instance.
>    //
>    NET_LIST_FOR_EACH_SAFE (Entry, Next, &MtftpSb->Children) {
>      Instance = NET_LIST_USER_STRUCT (Entry, MTFTP4_PROTOCOL, Link);
> -
> -    if ((Instance->PacketToLive == 0) || (--Instance->PacketToLive > 0))
> {
> +    if (!Instance->HasTimeout) {
>        continue;
>      }
> +
> +    Instance->HasTimeout = FALSE;
> 
>      //
>      // Call the user's time out handler
>      //
>      Token = Instance->Token;
> 
> -    if ((Token->TimeoutCallback != NULL) &&
> +    if (Token != NULL && Token->TimeoutCallback != NULL &&
>          EFI_ERROR (Token->TimeoutCallback (&Instance->Mtftp4, Token))) {
> -
>        Mtftp4SendError (
> -         Instance,
> -         EFI_MTFTP4_ERRORCODE_REQUEST_DENIED,
> -         (UINT8 *) "User aborted the transfer in time out"
> -         );
> +        Instance,
> +        EFI_MTFTP4_ERRORCODE_REQUEST_DENIED,
> +        (UINT8 *) "User aborted the transfer in time out"
> +        );
> 
>        Mtftp4CleanOperation (Instance, EFI_ABORTED);
>        continue;
>      }
> 
> diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.h
> b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.h
> index 802e3867db..fd8703a925 100644
> --- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.h
> +++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.h
> @@ -1,9 +1,9 @@
>  /** @file
>    Support routines for MTFTP.
> 
> -Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
>  http://opensource.org/licenses/bsd-license.php<BR>
> 
> @@ -185,10 +185,24 @@ Mtftp4SendError (
>  EFI_STATUS
>  Mtftp4Retransmit (
>    IN MTFTP4_PROTOCOL        *Instance
>    );
> 
> +/**
> +  The timer ticking function in TPL_NOTIFY level for the Mtftp service
> instance.
> +
> +  @param  Event                 The ticking event
> +  @param  Context               The Mtftp service instance
> +
> +**/
> +VOID
> +EFIAPI
> +Mtftp4OnTimerTickNotifyLevel (
> +  IN EFI_EVENT              Event,
> +  IN VOID                   *Context
> +  );
> +
>  /**
>    The timer ticking function for the Mtftp service instance.
> 
>    @param  Event                 The ticking event
>    @param  Context               The Mtftp service instance
> --
> 2.16.2.windows.1



  reply	other threads:[~2018-03-02  6:40 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-02  6:43 [Patch] MdeModulePkg/Mtftp4Dxe: Separate the timer ticking to calculate the packet live time Jiaxin Wu
2018-03-02  6:45 ` Fu, Siyuan [this message]
2018-03-02  7:40 ` Ye, Ting

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=B1FF2E9001CE9041BD10B825821D5BC58B449E02@SHSMSX103.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox