[ovs-dev,v2] treewide: Use packet batch APIs
diff mbox series

Message ID 20190901131004.GA18575@Nover
State New
Headers show
Series
  • [ovs-dev,v2] treewide: Use packet batch APIs
Related show

Commit Message

Paul Chaignon Sept. 1, 2019, 1:10 p.m. UTC
This patch replaces direct accesses to dp_packet_batch and dp_packet
internal components by the appropriate API calls.  It extends commit
1270b6e52 (treewide: Wider use of packet batch APIs).

This patch was generated using the following semantic patch (cf.
http://coccinelle.lip6.fr).

// <smpl>
@ dp_packet @
struct dp_packet_batch *b1;
struct dp_packet_batch b2;
struct dp_packet *p;
expression e;
@@

(
- b1->packets[b1->count++] = p;
+ dp_packet_batch_add(b1, p);
|
- b2.packets[b2.count++] = p;
+ dp_packet_batch_add(&b2, p);
|
- p->packet_type == htonl(PT_ETH)
+ dp_packet_is_eth(p)
|
- p->packet_type != htonl(PT_ETH)
+ !dp_packet_is_eth(p)
|
- b1->count == 0
+ dp_packet_batch_is_empty(b1)
|
- !b1->count
+ dp_packet_batch_is_empty(b1)
|
  b1->count = e;
|
  b1->count++
|
  b2.count = e;
|
  b2.count++
|
- b1->count
+ dp_packet_batch_size(b1)
|
- b2.count
+ dp_packet_batch_size(&b2)
)
// </smpl>

Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
---
Changelogs:
  Changes in v2:
    - Rebased on master branch.
    - Re-applied the semantic patch (new changes in lib/netdev-afxdp.c).

 lib/dpif-netdev.c  |  7 ++++---
 lib/flow.c         |  2 +-
 lib/netdev-afxdp.c | 20 ++++++++++++--------
 lib/netdev-dpdk.c  |  3 ++-
 lib/netdev-dummy.c |  2 +-
 lib/packets.c      |  4 ++--
 lib/pcap-file.c    |  2 +-
 7 files changed, 23 insertions(+), 17 deletions(-)

Comments

William Tu Sept. 3, 2019, 4:37 p.m. UTC | #1
On Sun, Sep 01, 2019 at 03:10:05PM +0200, Paul Chaignon wrote:
> This patch replaces direct accesses to dp_packet_batch and dp_packet
> internal components by the appropriate API calls.  It extends commit
> 1270b6e52 (treewide: Wider use of packet batch APIs).
> 
> This patch was generated using the following semantic patch (cf.
> http://coccinelle.lip6.fr).

Looks very interesting, I spent some time learning it.
If you have time, can you show us how to run it?
I installed coccinelle on ubuntu and I can run on linux kernel
 make coccicheck MODE=report
but for OVS, do we provide the semantic patch below and run
it manually?

> 
> // <smpl>
> @ dp_packet @
> struct dp_packet_batch *b1;
> struct dp_packet_batch b2;
> struct dp_packet *p;
> expression e;
> @@
> 
> (
> - b1->packets[b1->count++] = p;
> + dp_packet_batch_add(b1, p);
> |
> - b2.packets[b2.count++] = p;
> + dp_packet_batch_add(&b2, p);
> |
> - p->packet_type == htonl(PT_ETH)
> + dp_packet_is_eth(p)
> |
> - p->packet_type != htonl(PT_ETH)
> + !dp_packet_is_eth(p)
> |
> - b1->count == 0
> + dp_packet_batch_is_empty(b1)
> |
> - !b1->count
> + dp_packet_batch_is_empty(b1)

I understand the above using + and -
But can you explain the lines below?
> |
>   b1->count = e;
> |
>   b1->count++
> |
>   b2.count = e;
> |
>   b2.count++
> |
Why do we need "expression e" and are they match and replace something?

> - b1->count
> + dp_packet_batch_size(b1)
> |
> - b2.count
> + dp_packet_batch_size(&b2)
> )
> // </smpl>
> 
> Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>

Looks good to me, thanks for the patch.
Acked-by: William Tu <u9012063@gmail.com>


> ---
> Changelogs:
>   Changes in v2:
>     - Rebased on master branch.
>     - Re-applied the semantic patch (new changes in lib/netdev-afxdp.c).
> 
>  lib/dpif-netdev.c  |  7 ++++---
>  lib/flow.c         |  2 +-
>  lib/netdev-afxdp.c | 20 ++++++++++++--------
>  lib/netdev-dpdk.c  |  3 ++-
>  lib/netdev-dummy.c |  2 +-
>  lib/packets.c      |  4 ++--
>  lib/pcap-file.c    |  2 +-
>  7 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 75d85b2fd..d24f9502f 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -4261,7 +4261,7 @@ dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread *pmd,
>          /* At least one packet received. */
>          *recirc_depth_get() = 0;
>          pmd_thread_ctx_time_update(pmd);
> -        batch_cnt = batch.count;
> +        batch_cnt = dp_packet_batch_size(&batch);
>          if (pmd_perf_metrics_enabled(pmd)) {
>              /* Update batch histogram. */
>              s->current.batches++;
> @@ -6292,7 +6292,7 @@ packet_batch_per_flow_update(struct packet_batch_per_flow *batch,
>  {
>      batch->byte_count += dp_packet_size(packet);
>      batch->tcp_flags |= tcp_flags;
> -    batch->array.packets[batch->array.count++] = packet;
> +    dp_packet_batch_add(&batch->array, packet);
>  }
>  
>  static inline void
> @@ -6314,7 +6314,8 @@ packet_batch_per_flow_execute(struct packet_batch_per_flow *batch,
>      struct dp_netdev_actions *actions;
>      struct dp_netdev_flow *flow = batch->flow;
>  
> -    dp_netdev_flow_used(flow, batch->array.count, batch->byte_count,
> +    dp_netdev_flow_used(flow, dp_packet_batch_size(&batch->array),
> +                        batch->byte_count,
>                          batch->tcp_flags, pmd->ctx.now / 1000);
>  
>      actions = dp_netdev_flow_get_actions(flow);
> diff --git a/lib/flow.c b/lib/flow.c
> index ac6a4e121..393243309 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -1098,7 +1098,7 @@ parse_tcp_flags(struct dp_packet *packet)
>      ovs_be16 dl_type;
>      uint8_t nw_frag = 0, nw_proto = 0;
>  
> -    if (packet->packet_type != htonl(PT_ETH)) {
> +    if (!dp_packet_is_eth(packet)) {
>          return 0;
>      }
>  
> diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
> index e5b058d08..6e0180327 100644
> --- a/lib/netdev-afxdp.c
> +++ b/lib/netdev-afxdp.c
> @@ -765,7 +765,7 @@ free_afxdp_buf_batch(struct dp_packet_batch *batch)
>          addr = (uintptr_t)base & (~FRAME_SHIFT_MASK);
>          elems[i] = (void *)addr;
>      }
> -    umem_elem_push_n(xpacket->mpool, batch->count, elems);
> +    umem_elem_push_n(xpacket->mpool, dp_packet_batch_size(batch), elems);
>      dp_packet_batch_init(batch);
>  }
>  
> @@ -860,9 +860,11 @@ __netdev_afxdp_batch_send(struct netdev *netdev, int qid,
>      free_batch = check_free_batch(batch);
>  
>      umem = xsk_info->umem;
> -    ret = umem_elem_pop_n(&umem->mpool, batch->count, elems_pop);
> +    ret = umem_elem_pop_n(&umem->mpool, dp_packet_batch_size(batch),
> +                          elems_pop);
>      if (OVS_UNLIKELY(ret)) {
> -        atomic_add_relaxed(&xsk_info->tx_dropped, batch->count, &orig);
> +        atomic_add_relaxed(&xsk_info->tx_dropped, dp_packet_batch_size(batch),
> +                           &orig);
>          VLOG_WARN_RL(&rl, "%s: send failed due to exhausted memory pool.",
>                       netdev_get_name(netdev));
>          error = ENOMEM;
> @@ -870,10 +872,12 @@ __netdev_afxdp_batch_send(struct netdev *netdev, int qid,
>      }
>  
>      /* Make sure we have enough TX descs. */
> -    ret = xsk_ring_prod__reserve(&xsk_info->tx, batch->count, &idx);
> +    ret = xsk_ring_prod__reserve(&xsk_info->tx, dp_packet_batch_size(batch),
> +                                 &idx);
>      if (OVS_UNLIKELY(ret == 0)) {
> -        umem_elem_push_n(&umem->mpool, batch->count, elems_pop);
> -        atomic_add_relaxed(&xsk_info->tx_dropped, batch->count, &orig);
> +        umem_elem_push_n(&umem->mpool, dp_packet_batch_size(batch), elems_pop);
> +        atomic_add_relaxed(&xsk_info->tx_dropped, dp_packet_batch_size(batch),
> +                           &orig);
>          COVERAGE_INC(afxdp_tx_full);
>          afxdp_complete_tx(xsk_info);
>          kick_tx(xsk_info, dev->xdpmode);
> @@ -897,8 +901,8 @@ __netdev_afxdp_batch_send(struct netdev *netdev, int qid,
>          xsk_ring_prod__tx_desc(&xsk_info->tx, idx + i)->len
>              = dp_packet_size(packet);
>      }
> -    xsk_ring_prod__submit(&xsk_info->tx, batch->count);
> -    xsk_info->outstanding_tx += batch->count;
> +    xsk_ring_prod__submit(&xsk_info->tx, dp_packet_batch_size(batch));
> +    xsk_info->outstanding_tx += dp_packet_batch_size(batch);
>  
>      ret = kick_tx(xsk_info, dev->xdpmode);
>      if (OVS_UNLIKELY(ret)) {
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index bc20d6843..7f709ff3e 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2475,7 +2475,8 @@ netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
>          dpdk_do_tx_copy(netdev, qid, batch);
>          dp_packet_delete_batch(batch, true);
>      } else {
> -        __netdev_dpdk_vhost_send(netdev, qid, batch->packets, batch->count);
> +        __netdev_dpdk_vhost_send(netdev, qid, batch->packets,
> +                                 dp_packet_batch_size(batch));
>      }
>      return 0;
>  }
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index f0c0fbae8..95e1a329a 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -1108,7 +1108,7 @@ netdev_dummy_send(struct netdev *netdev, int qid OVS_UNUSED,
>          const void *buffer = dp_packet_data(packet);
>          size_t size = dp_packet_size(packet);
>  
> -        if (packet->packet_type != htonl(PT_ETH)) {
> +        if (!dp_packet_is_eth(packet)) {
>              error = EPFNOSUPPORT;
>              break;
>          }
> diff --git a/lib/packets.c b/lib/packets.c
> index 12053df57..5ec2a4beb 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -246,7 +246,7 @@ push_eth(struct dp_packet *packet, const struct eth_addr *dst,
>  {
>      struct eth_header *eh;
>  
> -    ovs_assert(packet->packet_type != htonl(PT_ETH));
> +    ovs_assert(!dp_packet_is_eth(packet));
>      eh = dp_packet_resize_l2(packet, ETH_HEADER_LEN);
>      eh->eth_dst = *dst;
>      eh->eth_src = *src;
> @@ -265,7 +265,7 @@ pop_eth(struct dp_packet *packet)
>      ovs_be16 ethertype;
>      int increment;
>  
> -    ovs_assert(packet->packet_type == htonl(PT_ETH));
> +    ovs_assert(dp_packet_is_eth(packet));
>      ovs_assert(l3 != NULL);
>  
>      if (l2_5) {
> diff --git a/lib/pcap-file.c b/lib/pcap-file.c
> index e4e92b8c2..f0cac8e0f 100644
> --- a/lib/pcap-file.c
> +++ b/lib/pcap-file.c
> @@ -282,7 +282,7 @@ ovs_pcap_write(struct pcap_file *p_file, struct dp_packet *buf)
>      struct pcaprec_hdr prh;
>      struct timeval tv;
>  
> -    ovs_assert(buf->packet_type == htonl(PT_ETH));
> +    ovs_assert(dp_packet_is_eth(buf));
>  
>      xgettimeofday(&tv);
>      prh.ts_sec = tv.tv_sec;
> -- 
> 2.17.1
>

Patch
diff mbox series

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 75d85b2fd..d24f9502f 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4261,7 +4261,7 @@  dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread *pmd,
         /* At least one packet received. */
         *recirc_depth_get() = 0;
         pmd_thread_ctx_time_update(pmd);
-        batch_cnt = batch.count;
+        batch_cnt = dp_packet_batch_size(&batch);
         if (pmd_perf_metrics_enabled(pmd)) {
             /* Update batch histogram. */
             s->current.batches++;
@@ -6292,7 +6292,7 @@  packet_batch_per_flow_update(struct packet_batch_per_flow *batch,
 {
     batch->byte_count += dp_packet_size(packet);
     batch->tcp_flags |= tcp_flags;
-    batch->array.packets[batch->array.count++] = packet;
+    dp_packet_batch_add(&batch->array, packet);
 }
 
 static inline void
@@ -6314,7 +6314,8 @@  packet_batch_per_flow_execute(struct packet_batch_per_flow *batch,
     struct dp_netdev_actions *actions;
     struct dp_netdev_flow *flow = batch->flow;
 
-    dp_netdev_flow_used(flow, batch->array.count, batch->byte_count,
+    dp_netdev_flow_used(flow, dp_packet_batch_size(&batch->array),
+                        batch->byte_count,
                         batch->tcp_flags, pmd->ctx.now / 1000);
 
     actions = dp_netdev_flow_get_actions(flow);
diff --git a/lib/flow.c b/lib/flow.c
index ac6a4e121..393243309 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1098,7 +1098,7 @@  parse_tcp_flags(struct dp_packet *packet)
     ovs_be16 dl_type;
     uint8_t nw_frag = 0, nw_proto = 0;
 
-    if (packet->packet_type != htonl(PT_ETH)) {
+    if (!dp_packet_is_eth(packet)) {
         return 0;
     }
 
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index e5b058d08..6e0180327 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -765,7 +765,7 @@  free_afxdp_buf_batch(struct dp_packet_batch *batch)
         addr = (uintptr_t)base & (~FRAME_SHIFT_MASK);
         elems[i] = (void *)addr;
     }
-    umem_elem_push_n(xpacket->mpool, batch->count, elems);
+    umem_elem_push_n(xpacket->mpool, dp_packet_batch_size(batch), elems);
     dp_packet_batch_init(batch);
 }
 
@@ -860,9 +860,11 @@  __netdev_afxdp_batch_send(struct netdev *netdev, int qid,
     free_batch = check_free_batch(batch);
 
     umem = xsk_info->umem;
-    ret = umem_elem_pop_n(&umem->mpool, batch->count, elems_pop);
+    ret = umem_elem_pop_n(&umem->mpool, dp_packet_batch_size(batch),
+                          elems_pop);
     if (OVS_UNLIKELY(ret)) {
-        atomic_add_relaxed(&xsk_info->tx_dropped, batch->count, &orig);
+        atomic_add_relaxed(&xsk_info->tx_dropped, dp_packet_batch_size(batch),
+                           &orig);
         VLOG_WARN_RL(&rl, "%s: send failed due to exhausted memory pool.",
                      netdev_get_name(netdev));
         error = ENOMEM;
@@ -870,10 +872,12 @@  __netdev_afxdp_batch_send(struct netdev *netdev, int qid,
     }
 
     /* Make sure we have enough TX descs. */
-    ret = xsk_ring_prod__reserve(&xsk_info->tx, batch->count, &idx);
+    ret = xsk_ring_prod__reserve(&xsk_info->tx, dp_packet_batch_size(batch),
+                                 &idx);
     if (OVS_UNLIKELY(ret == 0)) {
-        umem_elem_push_n(&umem->mpool, batch->count, elems_pop);
-        atomic_add_relaxed(&xsk_info->tx_dropped, batch->count, &orig);
+        umem_elem_push_n(&umem->mpool, dp_packet_batch_size(batch), elems_pop);
+        atomic_add_relaxed(&xsk_info->tx_dropped, dp_packet_batch_size(batch),
+                           &orig);
         COVERAGE_INC(afxdp_tx_full);
         afxdp_complete_tx(xsk_info);
         kick_tx(xsk_info, dev->xdpmode);
@@ -897,8 +901,8 @@  __netdev_afxdp_batch_send(struct netdev *netdev, int qid,
         xsk_ring_prod__tx_desc(&xsk_info->tx, idx + i)->len
             = dp_packet_size(packet);
     }
-    xsk_ring_prod__submit(&xsk_info->tx, batch->count);
-    xsk_info->outstanding_tx += batch->count;
+    xsk_ring_prod__submit(&xsk_info->tx, dp_packet_batch_size(batch));
+    xsk_info->outstanding_tx += dp_packet_batch_size(batch);
 
     ret = kick_tx(xsk_info, dev->xdpmode);
     if (OVS_UNLIKELY(ret)) {
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index bc20d6843..7f709ff3e 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2475,7 +2475,8 @@  netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
         dpdk_do_tx_copy(netdev, qid, batch);
         dp_packet_delete_batch(batch, true);
     } else {
-        __netdev_dpdk_vhost_send(netdev, qid, batch->packets, batch->count);
+        __netdev_dpdk_vhost_send(netdev, qid, batch->packets,
+                                 dp_packet_batch_size(batch));
     }
     return 0;
 }
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index f0c0fbae8..95e1a329a 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -1108,7 +1108,7 @@  netdev_dummy_send(struct netdev *netdev, int qid OVS_UNUSED,
         const void *buffer = dp_packet_data(packet);
         size_t size = dp_packet_size(packet);
 
-        if (packet->packet_type != htonl(PT_ETH)) {
+        if (!dp_packet_is_eth(packet)) {
             error = EPFNOSUPPORT;
             break;
         }
diff --git a/lib/packets.c b/lib/packets.c
index 12053df57..5ec2a4beb 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -246,7 +246,7 @@  push_eth(struct dp_packet *packet, const struct eth_addr *dst,
 {
     struct eth_header *eh;
 
-    ovs_assert(packet->packet_type != htonl(PT_ETH));
+    ovs_assert(!dp_packet_is_eth(packet));
     eh = dp_packet_resize_l2(packet, ETH_HEADER_LEN);
     eh->eth_dst = *dst;
     eh->eth_src = *src;
@@ -265,7 +265,7 @@  pop_eth(struct dp_packet *packet)
     ovs_be16 ethertype;
     int increment;
 
-    ovs_assert(packet->packet_type == htonl(PT_ETH));
+    ovs_assert(dp_packet_is_eth(packet));
     ovs_assert(l3 != NULL);
 
     if (l2_5) {
diff --git a/lib/pcap-file.c b/lib/pcap-file.c
index e4e92b8c2..f0cac8e0f 100644
--- a/lib/pcap-file.c
+++ b/lib/pcap-file.c
@@ -282,7 +282,7 @@  ovs_pcap_write(struct pcap_file *p_file, struct dp_packet *buf)
     struct pcaprec_hdr prh;
     struct timeval tv;
 
-    ovs_assert(buf->packet_type == htonl(PT_ETH));
+    ovs_assert(dp_packet_is_eth(buf));
 
     xgettimeofday(&tv);
     prh.ts_sec = tv.tv_sec;