Re: [dpdk-dev] [PATCH 2/3] net/virtio-user: avoid parsing process mappings

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 17-Sep-18 12:57 PM, Tiwei Bie wrote:
> On Mon, Sep 17, 2018 at 11:17:42AM +0100, Burakov, Anatoly wrote:
>> On 10-Sep-18 4:59 AM, Tiwei Bie wrote:
>>> On Fri, Sep 07, 2018 at 01:21:35PM +0100, Burakov, Anatoly wrote:
>>>> On 07-Sep-18 12:35 PM, Tiwei Bie wrote:
>>>>> On Fri, Sep 07, 2018 at 10:39:16AM +0100, Burakov, Anatoly wrote:
>>>>>> On 05-Sep-18 5:28 AM, Tiwei Bie wrote:
>>>>>>> Recently some memory APIs were introduced to allow users to
>>>>>>> get the file descriptor and offset for each memory segment.
>>>>>>> We can leverage those APIs to get rid of the /proc magic on
>>>>>>> memory table preparation in vhost-user backend.
>>>>>>>
>>>>>>> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
>>>>>>> ---
>>>>>>
>>>>>> <snip>
>>>>>>
>>>>>>> -	for (i = 0; i < num; ++i) {
>>>>>>> -		mr = &msg->payload.memory.regions[i];
>>>>>>> -		mr->guest_phys_addr = huges[i].addr; /* use vaddr! */
>>>>>>> -		mr->userspace_addr = huges[i].addr;
>>>>>>> -		mr->memory_size = huges[i].size;
>>>>>>> -		mr->mmap_offset = 0;
>>>>>>> -		fds[i] = open(huges[i].path, O_RDWR);
>>>>>>> +	if (rte_memseg_get_fd_offset_thread_unsafe(ms, &offset) < 0) {
>>>>>>> +		PMD_DRV_LOG(ERR, "Failed to get offset, ms=%p rte_errno=%d",
>>>>>>> +			ms, rte_errno);
>>>>>>> +		return -1;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	start_addr = (uint64_t)(uintptr_t)ms->addr;
>>>>>>> +	end_addr = start_addr + ms->len;
>>>>>>> +
>>>>>>> +	for (i = 0; i < wa->region_nr; i++) {
>>>>>>
>>>>>> There has to be a better way than to run this loop on every segment. Maybe
>>>>>> store last-used region, and only do a region look up if there's a mismatch?
>>>>>> Generally, in single-file segments mode, you'll get lots of segments from
>>>>>> one memseg list one after the other, so you will need to do a lookup once
>>>>>> memseg list changes, instead of on each segment.
>>>>>
>>>>> We may have holes in one memseg list due to dynamic free.
>>>>> Do you mean we just need to do rte_memseg_contig_walk()
>>>>> and we can assume that fds of the contiguous memegs will
>>>>> be the same?
>>>>
>>>> No, i didn't mean that.
>>>>
>>>> Whether or not you are in single-file segments mode, you still need to scan
>>>> each segment. However, you lose your state when you exit this function, and
>>>> thus have to look up the appropriate memory region (that matches your fd)
>>>> again, over and over. It would be good if you could store last-used memory
>>>> region somewhere, so that next time you come back into this function, if the
>>>> memseg has the same fd, you will not have to look it up.
>>>>
>>>> Something like this:
>>>>
>>>> struct walk_arg {
>>>> 	*last_used;
>>>> 	<snip>
>>>> }
>>>>
>>>> int walk_func() {
>>>> 	<snip>
>>>> 	cur_region = wa->last_used; // check if it matches
>>>> 	if (cur->region->fd != fd) {
>>>> 		// fd is different - we've changed the segment
>>>> 		<snip>
>>>> 		wa->last_used = cur_region
>>>> 	}
>>>> }
>>>
>>> Thanks for the code. :)
>>>
>>>>
>>>> So, cache last used region to not have to look it up again, because chances
>>>> are, you won't have to. That way, you will still do region lookups, but only
>>>> if you have to - not every time.
>>>
>>> I can do it, but I'm not sure this optimization is really
>>> necessary. Because this loop should be quite fast, as the
>>> max number of regions permitted by vhost-user is quite
>>> small. And actually we need to do that loop at least once
>>> for each packet in vhost-user's dequeue and enqueue path,
>>> i.e. the data path.
>>
>> The number of regions is small, but the number of segments may be in the
>> thousands. Think worst case - 8K segments in the 16th region
> 
> The number of regions permitted by vhost-user is 8.
> And most likely, we just have two regions as the
> single-file-segment mode is mandatory when using
> 2MB pages.



> 
>> - with my code,
>> you will execute only 16 iterations on first segment and use "last used" for
>> the rest of the segments,
> 
> We still need to do 8K iterations on the segments.

Yes, but not 8K times (up to) 8 regions. Anyway, it's your driver, up to 
you :)

> 
>> while with your code, it'll be 8K times 16 :)
> 
> IMO, what we really need is a way to reduce "8K",
> i.e. reduce the number of segments (which could
> be thousands currently) we need to parse.
> 
> And the loop should be faster than the function
> call to rte_memseg_get_fd_thread_unsafe() and
> rte_memseg_get_fd_offset_thread_unsafe() (which
> are also called for each segment).

Unfortunately, we cannot do that, because we cannot make any assumptions 
about underlying structure of fd's.

Technically, i could introduce an fd-centric walk function (i.e. so 
that, instead of per-memseg or per-contiguous area walk, you'd get a 
per-fd walk), but i really don't want to pollute the API with another 
walk function.

> 
>>
>> You'll have to clarify the "for each packet" part, not sure i follow.
> 
> Take the vhost-PMD as an example, when doing Rx burst
> and Tx burst, for each mbuf (i.e. packet), we need to
> do that loop at least once.

Ah, OK, i get you - if it's fast enough to use on the data path, it's 
fast enough for memory mapping code :) Fair enough.

> 
>>
>> -- 
>> Thanks,
>> Anatoly
> 


-- 
Thanks,
Anatoly