On Fri, Sep 12, 2014 at 12:28 AM, Tomas Henzl <thenzl@xxxxxxxxxx> wrote: > On 09/11/2014 06:39 PM, Kashyap Desai wrote: >> On Thu, Sep 11, 2014 at 4:50 PM, Tomas Henzl <thenzl@xxxxxxxxxx> wrote: >>> On 09/11/2014 11:02 AM, Kashyap Desai wrote: >>>>> -----Original Message----- >>>>> From: Vivek Goyal [mailto:vgoyal@xxxxxxxxxx] >>>>> Sent: Wednesday, September 10, 2014 9:17 PM >>>>> To: Tomas Henzl >>>>> Cc: Elliott, Robert (Server Storage); Sumit Saxena; >>>> linux-scsi@xxxxxxxxxxxxxxx; >>>>> martin.petersen@xxxxxxxxxx; hch@xxxxxxxxxxxxx; >>>>> jbottomley@xxxxxxxxxxxxx; Kashyap Desai; aradford@xxxxxxxxx; Michal >>>>> Schmidt; amirv@xxxxxxxxxxxx; Jens Axboe <axboe@xxxxxxxxx> >>>>> (axboe@xxxxxxxxx); scameron@xxxxxxxxxxxxxxxxxx >>>>> Subject: Re: [PATCH 04/11] megaraid_sas : Firmware crash dump feature >>>>> support >>>>> >>>>> On Wed, Sep 10, 2014 at 05:28:40PM +0200, Tomas Henzl wrote: >>>>>> On 09/10/2014 05:06 PM, Elliott, Robert (Server Storage) wrote: >>>>>>>> From: linux-scsi-owner@xxxxxxxxxxxxxxx [mailto:linux-scsi- >>>>>>>> owner@xxxxxxxxxxxxxxx] On Behalf Of Sumit Saxena >>>>>>>> >>>>>>>>> From: Tomas Henzl [mailto:thenzl@xxxxxxxxxx] >>>>>>>>> >>>>>>>>> With several controllers in a system this may take a lot memory, >>>>>>>>> could you also in case when a kdump kernel is running lower it, by >>>>>>>>> not using this feature? >>>>>>>>> >>>>>>>> Agreed, we will disable this feature for kdump kernel by adding >>>>>>>> "reset_devices" global varaiable. >>>>>>>> That check is required for only one place, throughout the code, >>>>>>>> this feature will remain disabled. Code snippet for the same- >>>>>>>> >>>>>>>> instance->crash_dump_drv_support = (!reset_devices) && >>>>>>>> crashdump_enable && >>>>>>>> instance->crash_dump_fw_support && >>>>>>>> instance->crash_dump_buf); >>>>>>>> if(instance->crash_dump_drv_support) { >>>>>>>> printk(KERN_INFO "megaraid_sas: FW Crash dump is >>>>>>>> supported\n"); >>>>>>>> megasas_set_crash_dump_params(instance, >>>>>>>> MR_CRASH_BUF_TURN_OFF); >>>>>>>> >>>>>>>> } else { >>>>>>>> .. >>>>>>>> } >>>>>>> Network drivers have been running into similar problems. >>>>>>> >>>>>>> There's a new patch from Amir coming through net-next to make >>>>>>> is_kdump_kernel() (in crash_dump.h) accessible to modules. >>>>>>> That may be a better signal than reset_devices that the driver >>>>>>> should use minimal resources. >>>>>>> >>>>>>> http://comments.gmane.org/gmane.linux.network/324737 >>>>>>> >>>>>>> I'm not sure about the logistics of a SCSI patch depending on a >>>>>>> net-next patch. >>>>>> Probably better to start with reset_devices and switch to >>>>>> is_kdump_kernel() later. >>>>>> This is not a discussion about reset_devices versus is_kdump_kernel, >>>>>> but while it looks good to have it distinguished - is the >>>>>> reset_devices actually used anywhere else than in kdump kernel? >>>>> I think usage of reset_devices for lowering memory footprint of driver >>>> is >>>>> plain wrong. It tells driver to only reset the device as BIOS might not >>>> have >>>>> done it right or we skipped BIOS completely. >>>>> >>>>> Using is_kdump_kernel() is also not perfect either but atleast better >>>> than >>>>> reset_devices. >>>> We will use is_kdump_kernel() not to enable this feature in Kdump kernel. >>> OK, just keep in mind that the is_kdump_kernel() is not yet in mainline. >> Sure I read that part, but when I check kernel source I found >> is_kdump_kernel() is available. >> >> http://lxr.free-electrons.com/source/include/linux/crash_dump.h#L55 >> Let's do one thing - We will submit a separate patch for this to avoid >> any confusion. >> >> >>>> MegaRaid Driver will not allocate Host memory (which is used to collect >>>> complete FW crash image) until and unless associated FW is crashed/IO >>>> timeout. >>> This is new for the driver? A previous reply stated that the driver will >>> allocate 1MB for each MR controller at the 'start of the day'. >>> If I misunderstood it somehow then nothing is needed. >> This is correct. 1MB DMA buffer per controller will be allocated >> irrespective of FW is crashed or not. >> When FW actually crash driver will go and try to allocate upto 512MB >> memory. I though you queried for 512 MB memory. > > Memory allocated for a crashkernel depends on system configuration, > values below 100MB are often used - the less the better. > Every MB counts. It is true that we don't need full feature set in kdump mode and as specially FW crash dump feature is better to disable in kdump mode. Couple of places megaraid_sas driver use reset_devices variable, so will continue to use that and will resend patch Later when we have better API like "is_kdump_kernel()" we can replace in whole megaraid_sas driver. > >> >>>> Driver do allocation only if required. Also, it will break the function >>>> immediately after memory allocation is failed and operate on available >>>> memory. >>>> >>>> To be on safer side, we can disable this feature in Kdump mode. >>>> >>>> ~ Kashyap >>>>> Thanks >>>>> Vivek >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in >>>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > -- Device Driver Developer @ Avagotech Kashyap D. Desai Note - my new email address kashyap.desai@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
