1. 03 Feb, 2021 10 commits
    • Jakub Kicinski's avatar
      Merge branch 'net-ipa-don-t-disable-napi-in-suspend' · 64b268e1
      Jakub Kicinski authored
      Alex Elder says:
      
      ====================
      net: ipa: don't disable NAPI in suspend
      
      This is version 2 of a series that reworks the order in which things
      happen during channel stop and suspend (and start and resume), in
      order to address a hang that has been observed during suspend.
      The introductory message on the first version of the series gave
      some history which is omitted here.
      
      The end result of this series is that we only enable NAPI and the
      I/O completion interrupt on a channel when we start the channel for
      the first time.  And we only disable them when stopping the channel
      "for good."  In other words, NAPI and the completion interrupt
      remain enabled while a channel is stopped for suspend.
      
      One comment on version 1 of the series suggested *not* returning
      early on success in a function, instead having both success and
      error paths return from the same point at the end of the function
      block.  This has been addressed in this version.
      
      In addition, this version consolidates things a little bit, but the
      net result of the series is exactly the same as version 1 (with the
      exception of the return fix mentioned above).
      
      First, patch 6 in the first version was a small step to make patch 7
      easier to understand.  The two have been combined now.
      
      Second, previous version moved (and for suspend/resume, eliminated)
      I/O completion interrupt and NAPI disable/enable control in separate
      steps (patches).  Now both are moved around together in patch 5 and
      6, which eliminates the need for the final (NAPI-only) patch.
      
      I won't repeat the patch summaries provided in v1:
        https://lore.kernel.org/netdev/20210129202019.2099259-1-elder@linaro.org/
      
      Many thanks to Willem de Bruijn for his thoughtful input.
      ====================
      
      Link: https://lore.kernel.org/r/20210201172850.2221624-1-elder@linaro.orgSigned-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      64b268e1
    • Alex Elder's avatar
      net: ipa: expand last transaction check · e6316920
      Alex Elder authored
      Transactions to send data for a network device can be allocated at
      any time up until the point the TX queue is stopped.  It is possible
      for ipa_start_xmit() to be called in one context just before a
      the transmit queue is stopped in another.
      
      Update gsi_channel_trans_last() so that for TX channels the
      allocated and pending transaction lists are checked--in addition
      to the completed and polled lists--to determine the "last"
      transaction.  This means any transaction that has been allocated
      before the TX queue is stopped will be allowed to complete before
      we conclude the channel is quiesced.
      
      Rework the function a bit to use a list pointer and gotos.
      Signed-off-by: default avatarAlex Elder <elder@linaro.org>
      Acked-by: default avatarWillem de Bruijn <willemb@google.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      e6316920
    • Alex Elder's avatar
      net: ipa: don't disable interrupt on suspend · a65c0288
      Alex Elder authored
      No completion interrupts will occur while an endpoint is suspended,
      nor when a channel has been stopped for suspend.  So there's no need
      to disable the interrupt during suspend and re-enable it when
      resuming.  Without any interrupts occurring, there is no need to
      disable/re-enable NAPI for channel suspend/resume either.
      
      We'll only enable NAPI and the interrupt when we first start the
      channel, and disable it again only when it's "really" stopped.
      
      To accomplish this, move the enable/disable calls out of
      __gsi_channel_start() and __gsi_channel_stop(), and into
      gsi_channel_start() and gsi_channel_stop() instead.
      
      Add a call to napi_synchronize() to gsi_channel_suspend(), to ensure
      NAPI polling is done before moving on.
      Signed-off-by: default avatarAlex Elder <elder@linaro.org>
      Acked-by: default avatarWillem de Bruijn <willemb@google.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      a65c0288
    • Alex Elder's avatar
      net: ipa: disable interrupt and NAPI after channel stop · 4fef691c
      Alex Elder authored
      Disable both the I/O completion interrupt and NAPI polling on a
      channel *after* we successfully stop it rather than before.  This
      ensures a completion occurring just before the channel is stopped
      gets processed.
      
      Enable NAPI polling and the interrupt *before* starting a channel
      rather than after, to be symmetric.  A stopped channel won't
      generate any completion interrupts anyway.
      
      Enable NAPI before the interrupt and disable it afterward.
      Signed-off-by: default avatarAlex Elder <elder@linaro.org>
      Acked-by: default avatarWillem de Bruijn <willemb@google.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      4fef691c
    • Alex Elder's avatar
      net: ipa: kill gsi_channel_freeze() and gsi_channel_thaw() · bd1ea1e4
      Alex Elder authored
      Open-code gsi_channel_freeze() and gsi_channel_thaw() in all callers
      and get rid of these two functions.  This is part of reworking the
      sequence of things done during channel suspend/resume and start/stop.
      Signed-off-by: default avatarAlex Elder <elder@linaro.org>
      Acked-by: default avatarWillem de Bruijn <willemb@google.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      bd1ea1e4
    • Alex Elder's avatar
      net: ipa: introduce __gsi_channel_start() · 893b838e
      Alex Elder authored
      Create a new function that does most of the work of starting a
      channel.  What's different is that it takes a flag indicating
      whether the channel should really be started or not.  Create
      another new function __gsi_channel_stop() that behaves similarly.
      
      IPA v3.5.1 implements suspend using a special SUSPEND endpoint
      setting.  If the endpoint is suspended when an I/O completes on the
      underlying GSI channel, a SUSPEND interrupt is generated.
      
      Newer versions of IPA do not implement the SUSPEND endpoint mode.
      Instead, endpoint suspend is implemented by simply stopping the
      underlying GSI channel.  In this case, a completing I/O on a
      *stopped* channel causes the SUSPEND interrupt condition.
      
      These new functions put all activity related to starting or stopping
      a channel (including "thawing/freezing" the channel) in one place,
      whether or not the channel is actually started or stopped.
      Signed-off-by: default avatarAlex Elder <elder@linaro.org>
      Acked-by: default avatarWillem de Bruijn <willemb@google.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      893b838e
    • Alex Elder's avatar
      net: ipa: introduce gsi_channel_stop_retry() · 697e834e
      Alex Elder authored
      Create a new helper function that encapsulates issuing a set of
      channel stop commands, retrying if appropriate, with a short delay
      between attempts.
      Signed-off-by: default avatarAlex Elder <elder@linaro.org>
      Acked-by: default avatarWillem de Bruijn <willemb@google.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      697e834e
    • Alex Elder's avatar
      net: ipa: don't thaw channel if error starting · 6b00a76a
      Alex Elder authored
      If an error occurs starting a channel, don't "thaw" it.
      We should assume the channel remains in a non-started state.
      
      Update the comment in gsi_channel_stop(); calls to this function
      are no longer retried.
      Signed-off-by: default avatarAlex Elder <elder@linaro.org>
      Acked-by: default avatarWillem de Bruijn <willemb@google.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      6b00a76a
    • Marco Elver's avatar
      net: fix up truesize of cloned skb in skb_prepare_for_shift() · 097b9146
      Marco Elver authored
      Avoid the assumption that ksize(kmalloc(S)) == ksize(kmalloc(S)): when
      cloning an skb, save and restore truesize after pskb_expand_head(). This
      can occur if the allocator decides to service an allocation of the same
      size differently (e.g. use a different size class, or pass the
      allocation on to KFENCE).
      
      Because truesize is used for bookkeeping (such as sk_wmem_queued), a
      modified truesize of a cloned skb may result in corrupt bookkeeping and
      relevant warnings (such as in sk_stream_kill_queues()).
      
      Link: https://lkml.kernel.org/r/X9JR/J6dMMOy1obu@elver.google.com
      Reported-by: syzbot+7b99aafdcc2eedea6178@syzkaller.appspotmail.com
      Suggested-by: default avatarEric Dumazet <edumazet@google.com>
      Signed-off-by: default avatarMarco Elver <elver@google.com>
      Signed-off-by: default avatarEric Dumazet <edumazet@google.com>
      Link: https://lore.kernel.org/r/20210201160420.2826895-1-elver@google.comSigned-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      097b9146
    • Davide Caratti's avatar
      mptcp: fix length of MP_PRIO suboption · ec99a470
      Davide Caratti authored
      With version 0 of the protocol it was legal to encode the 'Subflow Id' in
      the MP_PRIO suboption, to specify which subflow would change its 'Backup'
      flag. This has been removed from v1 specification: thus, according to RFC
      8684 §3.3.8, the resulting 'Length' for MP_PRIO changed from 4 to 3 byte.
      
      Current Linux generates / parses MP_PRIO according to the old spec, using
      'Length' equal to 4, and hardcoding 1 as 'Subflow Id'; RFC compliance can
      improve if we change 'Length' in other to become 3, leaving a 'Nop' after
      the MP_PRIO suboption. In this way the kernel will emit and accept *only*
      MP_PRIO suboptions that are compliant to version 1 of the MPTCP protocol.
      
       unpatched 5.11-rc kernel:
       [root@bottarga ~]# tcpdump -tnnr unpatched.pcap | grep prio
       reading from file unpatched.pcap, link-type LINUX_SLL (Linux cooked v1)
       dropped privs to tcpdump
       IP 10.0.3.2.48433 > 10.0.1.1.10006: Flags [.], ack 1, win 502, options [nop,nop,TS val 4032325513 ecr 1876514270,mptcp prio non-backup id 1,mptcp dss ack 14084896651682217737], length 0
      
       patched 5.11-rc kernel:
       [root@bottarga ~]# tcpdump -tnnr patched.pcap | grep prio
       reading from file patched.pcap, link-type LINUX_SLL (Linux cooked v1)
       dropped privs to tcpdump
       IP 10.0.3.2.49735 > 10.0.1.1.10006: Flags [.], ack 1, win 502, options [nop,nop,TS val 1276737699 ecr 2686399734,mptcp prio non-backup,nop,mptcp dss ack 18433038869082491686], length 0
      
      Changes since v2:
       - when accounting for option space, don't increment 'TCPOLEN_MPTCP_PRIO'
         and use 'TCPOLEN_MPTCP_PRIO_ALIGN' instead, thanks to Matthieu Baerts.
      Changes since v1:
       - refactor patch to avoid using 'TCPOLEN_MPTCP_PRIO' with its old value,
         thanks to Geliang Tang.
      
      Fixes: 06706542 ("mptcp: add the outgoing MP_PRIO support")
      Reviewed-by: default avatarMat Martineau <mathew.j.martineau@linux.intel.com>
      Reviewed-by: default avatarMatthieu Baerts <matthieu.baerts@tessares.net>
      Signed-off-by: default avatarDavide Caratti <dcaratti@redhat.com>
      Reviewed-by: default avatarMatteo Croce <mcroce@linux.microsoft.com>
      Link: https://lore.kernel.org/r/846cdd41e6ad6ec88ef23fee1552ab39c2f5a3d1.1612184361.git.dcaratti@redhat.comSigned-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      ec99a470
  2. 02 Feb, 2021 30 commits