From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 3ACF595A5 for ; Thu, 7 Jan 2016 03:27:10 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP; 06 Jan 2016 18:27:09 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,531,1444719600"; d="scan'208";a="629237035" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by FMSMGA003.fm.intel.com with ESMTP; 06 Jan 2016 18:27:08 -0800 Date: Thu, 7 Jan 2016 10:31:06 +0800 From: Yuanhan Liu To: Rich Lane Message-ID: <20160107023106.GI26062@yliu-dev.sh.intel.com> References: <1452032049-94324-1-git-send-email-rlane@bigswitch.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1452032049-94324-1-git-send-email-rlane@bigswitch.com> User-Agent: Mutt/1.5.23 (2014-03-12) Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] vhost: fix leak of fds and mmaps X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Jan 2016 02:27:10 -0000 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. Another note is that you should not name an internal function with "rte_" prefix; it should be reserved for public functions. --yliu