| Message ID | 20190901131004.GA18575@Nover |
|---|---|
| State | Accepted |
| Commit | 940ac2ce880349bf4d3bcb9c0571dbedadc8d769 |
| Headers | show |
| Series | [ovs-dev,v2] treewide: Use packet batch APIs | expand |
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 >
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). Thanks a lot. I applied this to master.
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;
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(-)