Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751702AbaKDKhu (ORCPT ); Tue, 4 Nov 2014 05:37:50 -0500 Received: from mail-wg0-f51.google.com ([74.125.82.51]:38722 "EHLO mail-wg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751125AbaKDKhp (ORCPT ); Tue, 4 Nov 2014 05:37:45 -0500 Message-ID: <5458AC75.3030207@plexistor.com> Date: Tue, 04 Nov 2014 12:37:41 +0200 From: Boaz Harrosh User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.0 MIME-Version: 1.0 To: "Wilcox, Matthew R" , "Elliott, Robert (Server Storage)" , Ross Zwisler , Jens Axboe , Nick Piggin , "Kani, Toshimitsu" , "Knippers, Linda" , "linux-fsdevel@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-nvdimm@lists.01.org" , Matthew Wilcox Subject: Re: [PATCH 1/4] pmem: Initial version of persistent memory driver References: <1409173922-7484-1-git-send-email-ross.zwisler@linux.intel.com> <1409173922-7484-2-git-send-email-ross.zwisler@linux.intel.com> <94D0CD8314A33A4D9D801C0FE68B4029593548AB@G9W0745.americas.hpqcorp.net> <100D68C7BA14664A8938383216E40DE04082985D@FMSMSX114.amr.corp.intel.com> In-Reply-To: <100D68C7BA14664A8938383216E40DE04082985D@FMSMSX114.amr.corp.intel.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/03/2014 06:19 PM, Wilcox, Matthew R wrote: <> >>> +config BLK_DEV_PMEM_COUNT >>> + int "Default number of PMEM disks" >>> + default "4" >> >> For real use I think a default of 1 would be better. > > For real use, you need at least two to run xfstests. This whole configuration mechanism is going away soon anyway. > >>> + size_t page_offset = sector >> PAGE_SECTORS_SHIFT; >>> + size_t offset = page_offset << PAGE_SHIFT; >>> + >> >> Since page_offset is only used to calculate offset, they >> could be joined to avoid relying on the compiler to >> optimize it away: >> size_t offset = sector >> PAGE_SECTORS_SHIFT << PAGE_SHIFT; > > If you insist on doing the compiler's job for it, why not: > > size_t offset = sector >> (PAGE_SECTORS_SHIFT - PAGE_SHIFT); > > I actually think the original is clearer. > I wish you guys would actually review the correct code. In the actual good driver that has any shape of proper code all these issue are gone. * config defaults gone, multiple-devices multiple-memory ranges fully supported hot plug style. * above shifts cruft completely gone it is left overs from brd.c and its page usage. * getgeo fixed to do what we realy want by the only application on earth that still uses it, fdisk. All other partitioners do not call it at all. Why are we reviewing dead code ? Cheers Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/