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!
>
>> 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.
>> @@ -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. It definitely has some overhead but should be minor.
Regards,
William
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
