[ovs-dev] [PATCH v2] treewide: Use packet batch APIs

Paul Chaignon paul.chaignon at orange.com
Tue Sep 3 17:39:00 UTC 2019


On Tue, Sep 03, 2019 at 06:37PM +0000, William Tu <u9012063 at gmail.com> wrote:
> 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.

Glad you find it interesting!  I'm actually trying to practice using it
myself.  (If you have any usage ideas for Open vSwitch, I'm interested; I
already went through all the treewide patches.)

> 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?

I executed the semantic patch below with:
  spatch --sp-file semantic-patch.cocci --very-quiet \
         --dir ./ > output.diff

Note that there will be some incorrect changes in lib/dp-packet.h which
you'll need to remove (e.g., Coccinelle tries to use dp_packet_is_eth()
inside dp_packet_is_eth()).  It's possible to avoid that, but afaik, only
at the cost of a slightly more verbose semantic patch.

>
> >
> > // <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?

If I were to remove the four following matches on count and keep only the
last two, Coccinelle would attempt to replace count assignments and
increments with dp_packet_batch_size().  I might end up with e.g.:

  - batch->count = 0;
  + dp_packet_batch_size(batch) = 0;

In short, I'm using these four matches to ensure the last two matches
replace only rvalues.

> > |
> >   b1->count = e;
> > |
> >   b1->count++
> > |
> >   b2.count = e;
> > |
> >   b2.count++
> > |
> Why do we need "expression e" and are they match and replace something?

The four preceding matches don't replace code as there's no -/+ in the
leftmost column.
Expression e represents anything that might be on the right-hand side.
The semantic patch doesn't run without it, I'm guessing because the
statement is incomplete.

>
> > - b1->count
> > + dp_packet_batch_size(b1)
> > |
> > - b2.count
> > + dp_packet_batch_size(&b2)
> > )
> > // </smpl>
> >
> > Signed-off-by: Paul Chaignon <paul.chaignon at orange.com>
>
> Looks good to me, thanks for the patch.
> Acked-by: William Tu <u9012063 at 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
> >


More information about the dev mailing list