On 1/7/2016 10:27 AM, Yuanhan Liu wrote: > On Tue, Jan 05, 2016 at 02:14:09PM -0800, Rich Lane wrote: >> The common vhost code only supported a single mmap per device. vhost-user >> worked around this by saving the address/length/fd of each mmap after the end >> of the rte_virtio_memory struct. This only works if the vhost-user code frees >> dev->mem, since the common code is unaware of the extra info. The >> VHOST_USER_RESET_OWNER message is one situation where the common code frees >> dev->mem and leaks the fds and mappings. This happens every time I shut down >> a >> VM. > That is a good catch! But I don't think it needs the complexity your > patch has to fix it. Besides that, your patch has ABI change, which > is not acceptable at this stage. > > Back to the issue, the thing is that, mmap/unmap is vhost-user/vhost-cuse > specific, thus we'd better to handle them inside vhost-user/vhost-cuse > differently, but not in the common path, virtio-net.c: let the common > path handle common things only. That would make the logic clear, and > hence less error prone. > > Note that we have already handled the mmap inside vhost-user/vhost-cuse, > thinking of that way, there is no reason to handle unmap at virtio-net.c: > it should be a historic issue while we added vhost-cuse first, which will > not be an issue until we added vhost-user.
Agree. Let vhost-user part handle its specific cleanup and let the virtio-net.c handle the common logic. > > Another note is that you should not name an internal function with "rte_" > prefix; it should be reserved for public functions. > > --yliu >
