From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755066AbbBDAfV (ORCPT ); Tue, 3 Feb 2015 19:35:21 -0500 Received: from mail-pa0-f47.google.com ([209.85.220.47]:56667 "EHLO mail-pa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754037AbbBDAfU (ORCPT ); Tue, 3 Feb 2015 19:35:20 -0500 Date: Wed, 4 Feb 2015 09:35:10 +0900 From: Minchan Kim To: Sergey Senozhatsky Cc: Sergey Senozhatsky , Andrew Morton , Ganesh Mahendran , Jerome Marchand , Nitin Gupta , linux-kernel@vger.kernel.org, nefelim4ag@gmail.com, eternaleye@gmail.com Subject: Re: [PATCH] zram: rework reset and destroy path Message-ID: <20150204003510.GA27787@blaptop> References: <1422980106-28222-1-git-send-email-sergey.senozhatsky@gmail.com> <20150203234201.GA3583@blaptop> <20150204002450.GA527@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150204002450.GA527@swordfish> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 04, 2015 at 09:24:51AM +0900, Sergey Senozhatsky wrote: > Hello Minchan, > > On (02/04/15 08:42), Minchan Kim wrote: > > > + kfree(zram_devices); > > > + unregister_blkdev(zram_major, "zram"); > > > + pr_debug("Destroyed %u device(s)\n", nr); > > > > Create_device just shows the number of created device so I think > > no worth to emit per-device information in destroy_devices. > > Let's just emit clean up done like old in zram_exit but > > use pr_info instead of pr_debug. > > not critical let's keep it as is (it just mirrors the message from init()), > and I wouldn't say it's totally useless now. we allocate space for devices, > disk, queue, etc. and we destroy it here. As I said, it is just minor but at least want to fix pr_debug into pr_info. > > please see below. > > > Another concern is I'd like to keep per-device interface(e,g. > > create_device, destroy_device) because there was requirement > > to add new zram device dynamically. I guess you could remember > > that. Although I didn't have a enough time to response, > > Alex finally convinced me so I hope a contributor who have time > > will do it if he has an interest about that. > > yes, I was going to tell you that perhaps I'll do that. I had several > discussions on google+ and it seems that people want to see this > feature. so I was going to ask your opinion. > > the solution I'm thinking about now is to replace zram devices fixed > size array with a list, protected by mutex or spin lock (doesn't > matter at this point). this will change destroy_devices() from array > iteration to destroy each list entry. Yeb. > > so: > a) pr_debug("Destroyed %u device(s)\n", nr) > it will show the actual number of active devices by that time. > > b) I'll refactor destroy_devices(). > > this rework will not make it into the upcoming merge window, so we'll > have enough time. > > I haven't decided yet, if I wan't to keep released zram devices in the > list (idle zram devices list, picking up the first available device > when user requests new zram device) or I will destroy abondoned devices > and , thus, the list will represent only active devices. I perfer destroy abandoned devices. > > > I tend to select the latter one -- destroy unused zram devices. I don't > want to give additional sysfs knob to limit the number of idle devices, > waste memory for idle devices, etc. We already have a number of quite > complicated knobs. True. -- Kind regards, Minchan Kim