1. 08 Jul, 2018 15 commits
    • David S. Miller's avatar
      Merge branch 'Modify-action-API-for-implementing-lockless-actions' · e9ec8045
      David S. Miller authored
      Vlad Buslov says:
      
      ====================
      Modify action API for implementing lockless actions
      
      Currently, all netlink protocol handlers for updating rules, actions and
      qdiscs are protected with single global rtnl lock which removes any
      possibility for parallelism. This patch set is a first step to remove
      rtnl lock dependency from TC rules update path.
      
      Recently, new rtnl registration flag RTNL_FLAG_DOIT_UNLOCKED was added.
      Handlers registered with this flag are called without RTNL taken. End
      goal is to have rule update handlers(RTM_NEWTFILTER, RTM_DELTFILTER,
      etc.) to be registered with UNLOCKED flag to allow parallel execution.
      However, there is no intention to completely remove or split rtnl lock
      itself. This patch set addresses specific problems in action API that
      prevents it from being executed concurrently. This patch set does not
      completely unlock rules or actions update path. Additional patch sets
      are required to refactor individual actions and filters update for
      parallel execution.
      
      As a preparation for executing TC rules update handlers without rtnl
      lock, action API code was audited to determine areas that assume
      external synchronization with rtnl lock and must be changed to allow
      safe concurrent access with following results:
      
      1. Action idr is already protected with spinlock. However, some code
         paths assume that idr state is not changes between several
         consecutive tcf_idr_* function calls.
      2. tc_action reference and bind counters are implemented as plain
         integers. They purpose was to allow single actions to be shared
         between multiple filters, not to provide means for concurrent
         modification.
      3. tc_action 'cookie' pointer field is not protected against
         modification.
      4. Action API functions, that work with set of actions, use intrusive
         linked list, which cannot be used concurrently without additional
         synchronization.
      5. Action API functions don't take reference to actions while using
         them, assuming external synchronization with rtnl lock.
      
      Following solutions to these problems are implemented:
      
      1. To remove assumption that idr state doesn't change between tcf_idr_*
         calls, implement new functions that atomically perform several
         operations on idr without releasing idr spinlock. (function to
         atomically lookup and delete action by index, function to atomically
         check if action exists and allocate new one if necessary, etc.)
      2. Use atomic operations on counters to make them suitable for
         concurrent get/put operations.
      3. Data that 'cookie' points to is never modified, so it enough to
         refactor it to rcu pointer to prevent concurrent de-allocation.
      4. Action API doesn't actually use any linked list specific operations
         on actions intrusive linked list, so it can be refactored to array in
         straightforward manner.
      5. Always take reference to action while accessing it in action API.
         tcf_idr_search function modified to take reference to action before
         returning it, so there is no way to lookup an action without
         incrementing its reference counter. All users of this function are
         modified to release the reference, after they done using action. With
         all users using reference counting, it is now safe to concurrently
         delete actions.
      
      Additionally, actions init function signature was expanded with
      'rtnl_held' argument, that allows actions that have internal dependency
      on rtnl lock to take/release it when necessary.
      
      Since only shared state in action API module are actions themselves and
      action idr, these changes are sufficient to not to rely on global rtnl
      lock for protection of internal action API data structures.
      
      Changes from V5 to V6:
      - Rebase on current net-next
      - When action is deleted, set pointer in actions array to NULL to
        prevent double freeing.
      
      Changes from V4 to V5:
      - Change action delete API to track actions that were deleted, to
        prevent releasing them on error.
      
      Changes from V3 to V4:
      - Expand cover letter.
      - Reduce actions array size in tcf_action_init_1.
      - Rebase on latest net-next.
      
      Changes from V2 to V3:
      - Re-send with changelog copied to individual patches.
      
      Changes from V1 to V2:
      - Removed redundant actions ops lookup during delete.
      - Merge action ops delete definition and implementation.
      - Assume all actions have delete implemented and don't check for it
        explicitly.
      - Resplit action lookup/release code to prevent memory leaks in
        individual patches.
      - Make __tcf_idr_check function static
      - Remove unique idr insertion function. Change original idr insert to do
        the same thing.
      - Merge changes that take reference to action when performing lookup and
        changes that account for this additional reference when dumping action
        to user space into single patch.
      - Change convoluted commit message.
      - Rename "unlocked" to "rtnl_held" for clarity.
      - Remove estimator lock add patch.
      - Refactor action check-alloc code into standalone function.
      - Rename tcf_idr_find_delete to tcf_idr_delete_index.
      - Rearrange variable definitions in tc_action_delete.
      - Add patch that refactors action API code to use array of pointers to
        actions instead of intrusive linked list.
      - Expand cover letter.
      ====================
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      e9ec8045
    • Vlad Buslov's avatar
      net: sched: change action API to use array of pointers to actions · 90b73b77
      Vlad Buslov authored
      Act API used linked list to pass set of actions to functions. It is
      intrusive data structure that stores list nodes inside action structure
      itself, which means it is not safe to modify such list concurrently.
      However, action API doesn't use any linked list specific operations on this
      set of actions, so it can be safely refactored into plain pointer array.
      
      Refactor action API to use array of pointers to tc_actions instead of
      linked list. Change argument 'actions' type of exported action init,
      destroy and dump functions.
      Acked-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      90b73b77
    • Vlad Buslov's avatar
      net: sched: atomically check-allocate action · 0190c1d4
      Vlad Buslov authored
      Implement function that atomically checks if action exists and either takes
      reference to it, or allocates idr slot for action index to prevent
      concurrent allocations of actions with same index. Use EBUSY error pointer
      to indicate that idr slot is reserved.
      
      Implement cleanup helper function that removes temporary error pointer from
      idr. (in case of error between idr allocation and insertion of newly
      created action to specified index)
      
      Refactor all action init functions to insert new action to idr using this
      API.
      Reviewed-by: default avatarMarcelo Ricardo Leitner <marcelo.leitner@gmail.com>
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Signed-off-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      0190c1d4
    • Vlad Buslov's avatar
      net: sched: use reference counting action init · cae422f3
      Vlad Buslov authored
      Change action API to assume that action init function always takes
      reference to action, even when overwriting existing action. This is
      necessary because action API continues to use action pointer after init
      function is done. At this point action becomes accessible for concurrent
      modifications, so user must always hold reference to it.
      
      Implement helper put list function to atomically release list of actions
      after action API init code is done using them.
      Reviewed-by: default avatarMarcelo Ricardo Leitner <marcelo.leitner@gmail.com>
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Signed-off-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      cae422f3
    • Vlad Buslov's avatar
      net: sched: don't release reference on action overwrite · 4e8ddd7f
      Vlad Buslov authored
      Return from action init function with reference to action taken,
      even when overwriting existing action.
      
      Action init API initializes its fourth argument (pointer to pointer to tc
      action) to either existing action with same index or newly created action.
      In case of existing index(and bind argument is zero), init function returns
      without incrementing action reference counter. Caller of action init then
      proceeds working with action, without actually holding reference to it.
      This means that action could be deleted concurrently.
      
      Change action init behavior to always take reference to action before
      returning successfully, in order to protect from concurrent deletion.
      Reviewed-by: default avatarMarcelo Ricardo Leitner <marcelo.leitner@gmail.com>
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Signed-off-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      4e8ddd7f
    • Vlad Buslov's avatar
      net: sched: implement reference counted action release · 16af6067
      Vlad Buslov authored
      Implement helper delete function that uses new action ops 'delete', instead
      of destroying action directly. This is required so act API could delete
      actions by index, without holding any references to action that is being
      deleted.
      
      Implement function __tcf_action_put() that releases reference to action and
      frees it, if necessary. Refactor action deletion code to use new put
      function and not to rely on rtnl lock. Remove rtnl lock assertions that are
      no longer needed.
      Reviewed-by: default avatarMarcelo Ricardo Leitner <marcelo.leitner@gmail.com>
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Signed-off-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      16af6067
    • Vlad Buslov's avatar
      net: sched: add 'delete' function to action ops · b409074e
      Vlad Buslov authored
      Extend action ops with 'delete' function. Each action type to implements
      its own delete function that doesn't depend on rtnl lock.
      
      Implement delete function that is required to delete actions without
      holding rtnl lock. Use action API function that atomically deletes action
      only if it is still in action idr. This implementation prevents concurrent
      threads from deleting same action twice.
      Reviewed-by: default avatarMarcelo Ricardo Leitner <marcelo.leitner@gmail.com>
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Signed-off-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      b409074e
    • Vlad Buslov's avatar
      net: sched: implement action API that deletes action by index · 2a2ea349
      Vlad Buslov authored
      Implement new action API function that atomically finds and deletes action
      from idr by index. Intended to be used by lockless actions that do not rely
      on rtnl lock.
      Reviewed-by: default avatarMarcelo Ricardo Leitner <marcelo.leitner@gmail.com>
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Signed-off-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      2a2ea349
    • Vlad Buslov's avatar
      net: sched: always take reference to action · 3f7c72bc
      Vlad Buslov authored
      Without rtnl lock protection it is no longer safe to use pointer to tc
      action without holding reference to it. (it can be destroyed concurrently)
      
      Remove unsafe action idr lookup function. Instead of it, implement safe tcf
      idr check function that atomically looks up action in idr and increments
      its reference and bind counters. Implement both action search and check
      using new safe function
      
      Reference taken by idr check is temporal and should not be accounted by
      userspace clients (both logically and to preserver current API behavior).
      Subtract temporal reference when dumping action to userspace using existing
      tca_get_fill function arguments.
      Reviewed-by: default avatarMarcelo Ricardo Leitner <marcelo.leitner@gmail.com>
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Signed-off-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      3f7c72bc
    • Vlad Buslov's avatar
      net: sched: implement unlocked action init API · 789871bb
      Vlad Buslov authored
      Add additional 'rtnl_held' argument to act API init functions. It is
      required to implement actions that need to release rtnl lock before loading
      kernel module and reacquire if afterwards.
      Reviewed-by: default avatarMarcelo Ricardo Leitner <marcelo.leitner@gmail.com>
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Signed-off-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      789871bb
    • Vlad Buslov's avatar
      net: sched: change type of reference and bind counters · 036bb443
      Vlad Buslov authored
      Change type of action reference counter to refcount_t.
      
      Change type of action bind counter to atomic_t.
      This type is used to allow decrementing bind counter without testing
      for 0 result.
      Reviewed-by: default avatarMarcelo Ricardo Leitner <marcelo.leitner@gmail.com>
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Signed-off-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      036bb443
    • Vlad Buslov's avatar
      net: sched: use rcu for action cookie update · eec94fdb
      Vlad Buslov authored
      Implement functions to atomically update and free action cookie
      using rcu mechanism.
      Reviewed-by: default avatarMarcelo Ricardo Leitner <marcelo.leitner@gmail.com>
      Signed-off-by: default avatarVlad Buslov <vladbu@mellanox.com>
      Signed-off-by: default avatarJiri Pirko <jiri@mellanox.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      eec94fdb
    • Yifeng Sun's avatar
      openvswitch: kernel datapath clone action · b2335040
      Yifeng Sun authored
      Add 'clone' action to kernel datapath by using existing functions.
      When actions within clone don't modify the current flow, the flow
      key is not cloned before executing clone actions.
      
      This is a follow up patch for this incomplete work:
      https://patchwork.ozlabs.org/patch/722096/
      
      v1 -> v2:
      Refactor as advised by reviewer.
      Signed-off-by: default avatarYifeng Sun <pkusunyifeng@gmail.com>
      Signed-off-by: default avatarAndy Zhou <azhou@ovn.org>
      Acked-by: default avatarPravin B Shelar <pshelar@ovn.org>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      b2335040
    • Randy Dunlap's avatar
      isdn/capi: fix defined but not used warnings · 20fbdc35
      Randy Dunlap authored
      Fix build warnings in drivers/isdn/capi/ when CONFIG_PROC_FS is not
      enabled by marking the unused functions as __maybe_unused.
      
      ../drivers/isdn/capi/capi.c:1324:12: warning: 'capi20_proc_show' defined but not used [-Wunused-function]
      ../drivers/isdn/capi/capi.c:1347:12: warning: 'capi20ncci_proc_show' defined but not used [-Wunused-function]
      ../drivers/isdn/capi/capidrv.c:2454:12: warning: 'capidrv_proc_show' defined but not used [-Wunused-function]
      Signed-off-by: default avatarRandy Dunlap <rdunlap@infradead.org>
      Cc: Karsten Keil <isdn@linux-pingi.de>
      Cc: isdn4linux@listserv.isdn4linux.de (subscribers-only)
      Cc: netdev@vger.kernel.org
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      20fbdc35
    • Randy Dunlap's avatar
      connector: fix defined but not used warning · d2af686c
      Randy Dunlap authored
      Fix a build warning in connector.c when CONFIG_PROC_FS is not enabled
      by marking the unused function as __maybe_unused.
      
      ../drivers/connector/connector.c:242:12: warning: 'cn_proc_show' defined but not used [-Wunused-function]
      Signed-off-by: default avatarRandy Dunlap <rdunlap@infradead.org>
      Cc: Evgeniy Polyakov <zbr@ioremap.net>
      Cc: netdev@vger.kernel.org
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      d2af686c
  2. 07 Jul, 2018 25 commits