From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965611AbbBDAYy (ORCPT ); Tue, 3 Feb 2015 19:24:54 -0500 Received: from mail-pa0-f43.google.com ([209.85.220.43]:55803 "EHLO mail-pa0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752549AbbBDAYv (ORCPT ); Tue, 3 Feb 2015 19:24:51 -0500 Date: Wed, 4 Feb 2015 09:24:51 +0900 From: Sergey Senozhatsky To: Minchan Kim Cc: Sergey Senozhatsky , Andrew Morton , Ganesh Mahendran , Jerome Marchand , Nitin Gupta , Sergey Senozhatsky , linux-kernel@vger.kernel.org, nefelim4ag@gmail.com, eternaleye@gmail.com Subject: Re: [PATCH] zram: rework reset and destroy path Message-ID: <20150204002450.GA527@swordfish> References: <1422980106-28222-1-git-send-email-sergey.senozhatsky@gmail.com> <20150203234201.GA3583@blaptop> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150203234201.GA3583@blaptop> 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 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. 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. 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 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. -ss > For it, per-device creating/destroy interface looks better. > https://lkml.org/lkml/2014/8/8/142 > Anyway, I cannot expect it happens sooner so I'm not strong > against your patch(ie, create_device, destroy_devices) > because I think we could do refactoring it when we need it. >