On Sat, May 9, 2015 at 11:18 AM, Jiri Pirko [off-list ref] wrote:

Sat, May 09, 2015 at 07:40:07PM CEST, sfeldma@gmail.com wrote:

quoted

From: Scott Feldman <redacted>

For rocker, support prepare-commit transaction model for setting attributes
(and for adding objects).  This requires rocker to preallocate memory
needed for the commit up front in the prepare phase.  Since rtnl_lock is
held between prepare-commit, store the allocated memory on a queue hanging
off of the rocker_port.  Also, in prepare phase, do everything right up to
calling into HW.  The same code paths are tranversed in the driver for both
prepare and commit phases.  In some cases, any state modified in the
prepare phase must be reverted before returning so the commit phase makes
the same decisions.

As a consequence of holding rtnl_lock in process context for all attr sets
(and obj adds), all memory is GFP_KERNEL allocated and we don't need to
busy spin waiting for the device to complete the command.  So the bulk of
this patch is simplifying the memory allocations to only use GFP_KERNEL and
to remove the nowait flag and busy spin loop.

Signed-off-by: Scott Feldman <redacted>
---
drivers/net/ethernet/rocker/rocker.c |  393 +++++++++++++++++++++++-----------
1 file changed, 271 insertions(+), 122 deletions(-)

...

quoted

+static void *__rocker_port_alloc(struct rocker_port *rocker_port,
+                               size_t size, gfp_t flags,
+                               void *(*alloc)(size_t size, gfp_t flags))
+{

you can omit alloc param here, since __rocker_port_alloc is called
always with kzalloc. Also, flags is always GFP_KERNEL, but I have no
problem in having that.

fixed for v7

also, __rocker_port_alloc sond to me like it allocates actual port.
Maybe "__rocker_per_port_alloc" or something like that?
(same goes to other functions of similar name)

It's now __rocker_port_mem_alloc.

quoted

+      struct list_head *elem = NULL;
+
+      /* If in transaction prepare phase, allocate the memory
+       * and enqueue it on a per-port list.  If in transaction
+       * commit phase, dequeue the memory from the per-port list
+       * rather than re-allocating the memory.  The idea is the
+       * driver code paths for prepare and commit are identical
+       * so the memory allocated in the prepare phase is the
+       * memory used in the commit phase.
+       */
+
+      switch (rocker_port->trans) {
+      case SWITCHDEV_TRANS_PREPARE:
+              elem = alloc(size + sizeof(*elem), flags);
+              if (!elem)
+                      return NULL;
+              list_add_tail(elem, &rocker_port->trans_mem);
+              break;
+      case SWITCHDEV_TRANS_COMMIT:
+              BUG_ON(list_empty(&rocker_port->trans_mem));
+              elem = rocker_port->trans_mem.next;
+              list_del_init(elem);
+              break;
+      case SWITCHDEV_TRANS_NONE:
+              elem = alloc(size + sizeof(*elem), flags);
+              if (elem)
+                      INIT_LIST_HEAD(elem);
+              break;

I must say I don't like propagating SWITCHDEV_TRANS_* this deep into
driver. Also I don't like storing it in rocker_port->trans. I believe
that is should be seen only by rocker_port_attr_set. Then functions with
proper params should be called inside driver.

Ok, switched to passing it as param for v7

...

quoted

static int rocker_cmd_exec(struct rocker *rocker,
                         struct rocker_port *rocker_port,
                         rocker_cmd_cb_t prepare, void *prepare_priv,
-                         rocker_cmd_cb_t process, void *process_priv,
-                         bool nowait)
+                         rocker_cmd_cb_t process, void *process_priv)
{
      struct rocker_desc_info *desc_info;
      struct rocker_wait *wait;
      unsigned long flags;
      int err;

-      wait = rocker_wait_create(nowait ? GFP_ATOMIC : GFP_KERNEL);
+      wait = rocker_wait_create(rocker_port);
      if (!wait)
              return -ENOMEM;
-      wait->nowait = nowait;

      spin_lock_irqsave(&rocker->cmd_ring_lock, flags);
+

  ^^^

quoted

      desc_info = rocker_desc_head_get(&rocker->cmd_ring);
      if (!desc_info) {
              spin_unlock_irqrestore(&rocker->cmd_ring_lock, flags);
              err = -EAGAIN;
              goto out;
      }
+

  ^^^

quoted

      err = prepare(rocker, rocker_port, desc_info, prepare_priv);
      if (err) {
              spin_unlock_irqrestore(&rocker->cmd_ring_lock, flags);
              goto out;
      }
+

  ^^^ not sure why you adding these lines.

Readability: put some white space between logical chunks.