On Sun, Jun 26, 2016 at 9:54 PM, William Tu <u9012...@gmail.com> wrote:
> Hi Darrell,
>
> Thanks for you feedback!
>
> On Sun, Jun 26, 2016 at 8:02 PM, Darrell Ball <dlu...@gmail.com> wrote:
> >
> >
> > On Sat, Jun 25, 2016 at 7:44 AM, William Tu <u9012...@gmail.com> wrote:
> >>
> >> Commit 1895cc8dbb64 ("dpif-netdev: create batch object") introduces
> >> batch process functions and 'struct dp_packet_batch' to associate with
> >> batch-level metadata. This patch applies the packet batch object to
> >> the netdev provider interface (dummy, Linux, BSD, and DPDK) so that
> >> batch APIs can be used in providers. With batch metadata visible in
> >> providers, optimizations can be introduced at per-batch level instead
> >> of per-packet.
> >>
> >> Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/140135888
> >
> >
> > The test shows as failed ?
>
> it's due to one of the OVN testcases, which sometimes fails.
>
> >
> > The code change is replacing an array of ptrs to packets and count with a
> > packet batch and replacing some code blocks with equivalent existing
> inline
> > functions based on batchs.
> >
> > I have some comments inline.
> >
> >> @@ -681,10 +682,12 @@ netdev_bsd_rxq_drain(struct netdev_rxq *rxq_)
> >> */
> >> static int
> >> netdev_bsd_send(struct netdev *netdev_, int qid OVS_UNUSED,
> >> - struct dp_packet **pkts, int cnt, bool may_steal)
> >> + struct dp_packet_batch *batch, bool may_steal)
> >> {
> >> struct netdev_bsd *dev = netdev_bsd_cast(netdev_);
> >> const char *name = netdev_get_name(netdev_);
> >> + int cnt = batch->count;
> >> + struct dp_packet **okts = batch->packets;
> >
> >
> >
> > "okts" is not used in the function; "pkts" is used in the function.
> > The function will no longer work.
>
> my mistake, will fix it in next version. thanks!
>
Please check that it builds and test
>
> >
> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> >> index ed14a21..3b1e7fa 100644
> >> --- a/lib/netdev-dpdk.c
> >> +++ b/lib/netdev-dpdk.c
> >> @@ -1251,12 +1251,14 @@ netdev_dpdk_vhost_update_rx_counters(struct
> >> netdev_stats *stats,
> >> */
> >> static int
> >> netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
> >> - struct dp_packet **packets, int *c)
> >> + struct dp_packet_batch *batch)
> >> {
> >> struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev);
> >> struct virtio_net *virtio_dev = netdev_dpdk_get_virtio(dev);
> >> int qid = rxq->queue_id;
> >> struct ingress_policer *policer =
> >> netdev_dpdk_get_ingress_policer(dev);
> >> + struct dp_packet **packets = batch->packets;
> >> + int *c = &batch->count;
> >
> >
> > I see this pattern throughout where extra variables are used in all
> > functions.
> > There are 2 local variables for packets and count and also the batch
> > parameter,
> > which itself already has reference to packets and count from the start of
> > the call chain
> > in netdev_*().
> >
> > Pre-change, there are two parameters for packets and count derived from
> the
> > batch
> > parameter at the start of the call chain in netdev_*().
> >
> > These functions are intended to be fast.
>
> I don't think adding these 2 extra local variables will have overhead,
> I believe it's the same because compiler will optimize it.
>
Please don't make that general assumption
>
> >> @@ -1517,7 +1519,12 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid,
> >> struct dp_packet **pkts,
> >> }
> >>
> >> if (dev->type == DPDK_DEV_VHOST) {
> >> - __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **)
> >> mbufs, newcnt, true);
> >> + struct dp_packet_batch mbatch;
> >> +
> >> + dp_packet_batch_init(&mbatch);
> >> + mbatch.count = newcnt;
> >> + memcpy(mbatch.packets, mbufs, newcnt * sizeof(struct rte_mbuf
> >> *));
> >>
> >> + __netdev_dpdk_vhost_send(netdev, qid, &mbatch, true);
> >
> >
> > This looks more expensive to me in terms of processing performance in
> this
> > caller especially and also the callee.
> > This code is intended to be fast.
>
> This isn't copying the packet contents but only the array of 8byte
> pointers.
8 bytes vs 4 bytes vs ... depends on your system arch.
> It definitely has some overhead but should be minor.
>
To avoid any further confusion, below is the datastructure,
where the packet pointer array is sized at 32, today.
So it is a 256 bytes (for 8 byte addresses) allocation and memcpy
for each batch.
enum { NETDEV_MAX_BURST = 32 }
struct dp_packet_batch {
int count;
bool trunc; /* true if the batch needs truncate. */
>>>> struct dp_packet *packets[NETDEV_MAX_BURST]; <<<<
};
The point is this is extra overhead for code that tries to be fast.
>
> Regards,
> William
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
