From mboxrd@z Thu Jan  1 00:00:00 1970
From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Subject: Re: [PATCH v2 1/1] vhost: fix leak of fds and mmaps
Date: Sat, 6 Feb 2016 13:23:20 +0800
Message-ID: <20160206052320.GJ22958@yliu-dev.sh.intel.com>
References: <1452032049-94324-1-git-send-email-rlane@bigswitch.com>
 <1453060638-58510-1-git-send-email-rlane@bigswitch.com>
 <20160118075808.GZ19531@yliu-dev.sh.intel.com>
 <CAGSMBPN=5qemS6Ah4x5jUeqcULVWCaStOXp+0OA0mcBKGEHqEg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: dev@dpdk.org
To: Rich Lane <rich.lane@bigswitch.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga02.intel.com (mga02.intel.com [134.134.136.20])
 by dpdk.org (Postfix) with ESMTP id C4CB495D9
 for <dev@dpdk.org>; Sat,  6 Feb 2016 06:21:09 +0100 (CET)
Content-Disposition: inline
In-Reply-To: <CAGSMBPN=5qemS6Ah4x5jUeqcULVWCaStOXp+0OA0mcBKGEHqEg@mail.gmail.com>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hey Rich,

Sorry for the long delay; I barely forgot it :(

On Tue, Jan 19, 2016 at 10:13:23AM -0800, Rich Lane wrote:
> On Sun, Jan 17, 2016 at 11:58 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com>
> wrote:
> 
>     On Sun, Jan 17, 2016 at 11:57:18AM -0800, Rich Lane wrote:
>     > +/*
>     > + * Implementation-specific cleanup. Defined by vhost-cuse and
>     vhost-user.
>     > + */
>     > +void vhost_impl_cleanup(struct virtio_net *dev);
> 
>     TBH, I am not quite like "_impl_"; maybe "_backend_" is better?
> 
> 
> If you have a strong preference I will change it. Let me know.

"backend" is just a more common word to me, as well as to QEMU.
So, I would suggest you to do such change, and if so, you could 
add my ACK:

Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

> 
>     OTOH, what I thought of has slight difference than yours: not
>     necessary to export a function, but instead, call the vhost
>     backend specific unmap function inside the backend itself. Say,
>     call vhost_user_unmap() on RESET_OWNER and connection close.
>     What do you think of that?
> 
> 
> The munmap must be done after the notify_ops->destroy_device callback. That
> means
> the backend can't call it before reset_owner() or destroy_device().

Well, you could:

	case VHOST_USER_RESET_OWNER:
		ops->reset_owner();
		vhost_user_unmap();
		break;

Anyway, it's not a big deal. Let's go with your solution first.

	--yliu

> The munmap
> could
> be done afterwards, but that requires saving dev->mem in the caller in the case
> of
> destroy_device. The cleanest solution is for the vhost common code to ask the
> backend to clean up at the correct time.