> -----Original Message-----
> From: Robin Murphy [mailto:robin.mur...@arm.com]
> Sent: Saturday, October 31, 2020 4:48 AM
> To: Song Bao Hua (Barry Song) <song.bao....@hisilicon.com>;
> iommu@lists.linux-foundation.org; h...@lst.de; m.szyprow...@samsung.com
> Cc: j...@8bytes.org; w...@kernel.org; sh...@kernel.org; Linuxarm
> <linux...@huawei.com>; linux-kselft...@vger.kernel.org
> Subject: Re: [PATCH 1/2] dma-mapping: add benchmark support for streaming
> DMA APIs
>
> On 2020-10-29 21:39, Song Bao Hua (Barry Song) wrote:
> [...]
> >>> +struct map_benchmark {
> >>> + __u64 map_nsec;
> >>> + __u64 unmap_nsec;
> >>> + __u32 threads; /* how many threads will do map/unmap in parallel
> */
> >>> + __u32 seconds; /* how long the test will last */
> >>> + int node; /* which numa node this benchmark will run on */
> >>> + __u64 expansion[10]; /* For future use */
> >>> +};
> >>
> >> I'm no expert on userspace ABIs (and what little experience I do have
> >> is mostly of Win32...), so hopefully someone else will comment if
> >> there's anything of concern here. One thing I wonder is that there's
> >> a fair likelihood of functionality evolving here over time, so might
> >> it be appropriate to have some sort of explicit versioning parameter
> >> for robustness?
> >
> > I copied that from gup_benchmark. There is no this kind of code to
> > compare version.
> > I believe there is a likelihood that kernel module is changed but
> > users are still using old userspace tool, this might lead to the
> > incompatible data structure.
> > But not sure if it is a big problem :-)
>
> Yeah, like I say I don't really have a good feeling for what would be best
> here,
> I'm just thinking of what I do know and wary of the potential for a "640 bits
> ought to be enough for anyone" issue ;)
>
> >>> +struct map_benchmark_data {
> >>> + struct map_benchmark bparam;
> >>> + struct device *dev;
> >>> + struct dentry *debugfs;
> >>> + atomic64_t total_map_nsecs;
> >>> + atomic64_t total_map_loops;
> >>> + atomic64_t total_unmap_nsecs;
> >>> + atomic64_t total_unmap_loops;
> >>> +};
> >>> +
> >>> +static int map_benchmark_thread(void *data) {
> >>> + struct page *page;
> >>> + dma_addr_t dma_addr;
> >>> + struct map_benchmark_data *map = data;
> >>> + int ret = 0;
> >>> +
> >>> + page = alloc_page(GFP_KERNEL);
> >>> + if (!page)
> >>> + return -ENOMEM;
> >>> +
> >>> + while (!kthread_should_stop()) {
> >>> + ktime_t map_stime, map_etime, unmap_stime, unmap_etime;
> >>> +
> >>> + map_stime = ktime_get();
> >>> + dma_addr = dma_map_page(map->dev, page, 0, PAGE_SIZE,
> >> DMA_BIDIRECTIONAL);
> >>
> >> Note that for a non-coherent device, this will give an underestimate
> >> of the real-world overhead of BIDIRECTIONAL or TO_DEVICE mappings,
> >> since the page will never be dirty in the cache (except possibly the
> >> very first time through).
> >
> > Agreed. I'd like to add a DIRECTION parameter like "-d 0", "-d 1"
> > after we have this basic framework.
>
> That wasn't so much about the direction itself, just that if it's anything
> other
> than FROM_DEVICE, we should probably do something to dirty the buffer by a
> reasonable amount before each map. Otherwise the measured performance is
> going to be unrealistic on many systems.
Maybe put a memset(buf, 0, PAGE_SIZE) before dma_map will help ?
>
> [...]
> >>> + atomic64_add((long long)ktime_to_ns(ktime_sub(unmap_etime,
> >> unmap_stime)),
> >>> + &map->total_unmap_nsecs);
> >>> + atomic64_inc(&map->total_map_loops);
> >>> + atomic64_inc(&map->total_unmap_loops);
> >>
> >> I think it would be worth keeping track of the variances as well - it
> >> can be hard to tell if a reasonable-looking average is hiding
> >> terrible worst-case behaviour.
> >
> > This is a sensible requirement. I believe it is better to be handled
> > by the existing kernel tracing method.
> >
> > Maybe we need a histogram like:
> > Delay sample count
> > 1-2us 1000 ***
> > 2-3us 2000 *******
> > 3-4us 100 *
> > .....
> > This will be more precise than the maximum latency in the worst case.
> >
> > I'd believe this can be handled by:
> > tracepoint A
> > Map
> > Tracepoint B
> >
> > Tracepoint C
> > Unmap
> > Tracepoint D
> >
> > Let the userspace ebpf to draw the histogram for the delta of B-A and D-C.
> >
> > So I am planning to put this requirement into todo list and write an
> > userspace ebpf/bcc script for histogram and put in tools/ directory.
> >
> > Please give your comments on this.
>
> Right, I wasn't suggesting trying to homebrew a full data collection system
> here
> - I agree there are better tools for that already - just that it's basically
> free to
> track a sum of squares alongside a sum, so that we can trivially calculate a
> useful variance (or standard
> deviation) figure alongside the mean at the end.
For this case, I am not sure if it is true. Unless we expose more data such as
min, max etc. to userspace, it makes no difference whether total_(un)map_nsecs
and total_(un)map_loops are exposed or not.
As
total loops = seconds / (avg_map_latency + avg_unmap_latency);
total_map_nsecs = total loop count * avg_map_latency
total_unmap_nsecs = total loop count * avg_unmap_latency
all of seconds, avg_unmap_latency, avg_unmap_latency are known by
userspace tool.
>
> [...]
> >>> + for (i = 0; i < threads; i++) {
> >>> + tsk[i] = kthread_create_on_node(map_benchmark_thread, map,
> >>> + map->bparam.node, "dma-map-benchmark/%d", i);
> >>> + if (IS_ERR(tsk[i])) {
> >>> + dev_err(map->dev, "create dma_map thread failed\n");
> >>> + return PTR_ERR(tsk[i]);
> >>> + }
> >>> +
> >>> + if (node != NUMA_NO_NODE && node_online(node))
> >>> + kthread_bind_mask(tsk[i], cpu_mask);
> >>> +
> >>> + wake_up_process(tsk[i]);
> >>
> >> Might it be better to create all the threads first, *then* start
> >> kicking them?
> >
> > The difficulty is that we don't know how many threads we should create
> > as the thread number is a parameter to test the contention of IOMMU driver.
> > In my test case, I'd like to test things like One thread Two threads
> > ....
> > 8 threads
> > 12 threads
> > 16 threads...
> >
> > On the other hand, I think it is better to drop the memory of
> > task_struct of those test threads while we are not testing dma map.
>
> I simply meant splitting the loop here into two - one to create the threads
> and
> set their affinity, then another to wake them all up - so we don't start
> unnecessarily thrashing the system while we're still trying to set up the
> rest of
> the test ;)
Agreed.
>
> Robin.
Thanks
Barry
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
