Re: [PATCH net-next 4/8] enic: alloc/free rx_cpu_rmap

Hello.

On 06/10/2014 05:52 PM, Govindarajulu Varadarajan wrote:

quoted

quoted

@@ -1192,6 +1195,33 @@ static void enic_calc_int_moderation(struct enic

*enic, struct vnic_rq *rq)
      pkt_size_counter->small_pkt_bytes_cnt = 0;
  }

+#ifdef CONFIG_RFS_ACCEL
+static void enic_free_rx_cpu_rmap(struct enic *enic)
+{
+    free_irq_cpu_rmap(enic->netdev->rx_cpu_rmap);
+    enic->netdev->rx_cpu_rmap = NULL;
+}
+
+static inline void enic_set_rx_cpu_rmap(struct enic *enic)

quoted

  No need to use *inline* in a .c file, the compiler should figure it out.

Yes, I agree.

quoted

quoted

+{
+    int i, res;
+
+    if (vnic_dev_get_intr_mode(enic->vdev) == VNIC_DEV_INTR_MODE_MSIX) {
+        enic->netdev->rx_cpu_rmap = alloc_irq_cpu_rmap(enic->rq_count);
+        if (unlikely(!enic->netdev->rx_cpu_rmap))
+            return;
+        for (i = 0; i < enic->rq_count; i++) {
+            res = irq_cpu_rmap_add(enic->netdev->rx_cpu_rmap,
+                           enic->msix_entry[i].vector);
+            if (unlikely(res)) {
+                enic_free_rx_cpu_rmap(enic);
+                return;
+            }
+        }
+    }
+}

quoted

  It's better to do the following here:

quoted

#else
static void enic_free_rx_cpu_rmap(struct enic *enic)
{
}
static void enic_set_rx_cpu_rmap(struct enic *enic)
{
}

How about

static void enic_free_rx_cpu_rmap(struct enic *enic)
{
#ifdef CONFIG_RFS_ACCEL
...
...
#endif
}

I prefer this over yours because, if I use yours tools like cscope finds two
definitions of function enic_free_rx_cpu_rmap. Which makes code walk through
little bit difficult.

    #ifdef's in the function bodies are generally frowned upon. See 
Documentation/SubmittingPatches section 2 item (2).

Thanks
Govind

quoted

quoted

+#endif
+
  static int enic_poll_msix(struct napi_struct *napi, int budget)
  {
      struct net_device *netdev = napi->dev;

@@ -1267,6 +1297,9 @@ static void enic_free_intr(struct enic *enic)
      struct net_device *netdev = enic->netdev;
      unsigned int i;

+#ifdef CONFIG_RFS_ACCEL
+    enic_free_rx_cpu_rmap(enic);
+#endif

quoted

  ... so that you can avoid #ifdef's at the call sites.

WBR, Sergei