From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [RFC] KEXEC: allocate crash note buffers at boot time v3 Date: Thu, 1 Dec 2011 15:02:58 +0000 Message-ID: <4ED79722.7090404@citrix.com> References: <4ED62C50.7060204@citrix.com> <4ED666D5.5060603@citrix.com> <4ED7522E0200007800064973@nat28.tlf.novell.com> <4ED74DAD.1040903@citrix.com> <4ED75E8402000078000649E6@nat28.tlf.novell.com> <4ED77347.6060200@citrix.com> <4ED787870200007800064B6F@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4ED787870200007800064B6F@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: Keir Fraser , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 01/12/11 12:56, Jan Beulich wrote: >>>> On 01.12.11 at 13:29, Andrew Cooper wrote: >> +static spinlock_t crash_notes_lock = SPIN_LOCK_UNLOCKED; > Please use DEFINE_SPINLOCK() here. > >> + register_keyhandler('C', &crashdump_trigger_keyhandler); >> + >> + /* If no crash area, no need to allocate space for notes. */ >> + if ( 0 == kexec_crash_area.size ) >> + return 0; > Wouldn't it make sense to switch the order of these? > >> + crash_notes = xmalloc_bytes(nr_cpu_ids * sizeof(void*)); > Please use xmalloc_array() here. > >> + if ( !crash_notes[nr] && 0 != kexec_init_cpu_notes(nr) ) > The first check is pointless - the function will return zero if the > allocation was already done. I forgot to say in my previous email. Short circuit evaluation should prevent the kexec_init_cpu_notes() function call actually being made. > Further, you shouldn't take a lock around a call to xmalloc() or alike > unless absolutely necessary. It is pretty simple to avoid here - you > really only need to lock around the storing of the pointer and maybe > the setup_note() calls (but be careful with returning -ENOMEM - you > shouldn't if the allocation fails, but you then find - under the lock - > that a pointer was already set by another CPU). > > Finally, one thing I failed to notice on the previous version - the > nr_bytes calculations are now being done twice. This should > probably be moved into a helper function, especially since you > said you intend to add stuff here subsequently. > > Jan > -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com