[PATCH net-next 0/3] Make is_kdump_kernel() accessible from modules

[PATCH net-next 0/3] Make is_kdump_kernel() accessible from modules

[Amir Vadai]

Hi,

I'm sending this patchset to net-next because it touches a commit [1] that was
applied lately to net-next.

Drivers need to know if running from kdump kernel in order to change their
memory profile - since kdump environment is limited by available memory.
Currently there are drivers that are using reset_devices as suggested in [2].
In [2] it was suggested to use reset_devices, but the context was, to enable
driver to know when the hardware device is needed to be reset, and not if this
is a kdump environment. We think that is_kdump_kernel() is better suited to
select between different memory profiles.

The first patch in this patchset exports a needed symbol in order to make
is_kdump_kernel() accessible from the drivers. The rest of the patches change
from reset_devices to is_kdump_kernel() in 2 networking drivers.

The idea of this patchset was suggested by Vivek Goyal.

Tested and applied on top of commit f6e6753 ("Merge branch 'filter-move'")

[1] - ea1c1af: ("net/mlx4_en: Reduce memory consumption on kdump kernel")
[2] - https://lkml.org/lkml/2011/1/27/341

Thanks,
Amir

Amir Vadai (3):
  crash_dump: Make is_kdump_kernel() accessible from modules
  net/mlx4: Use is_kdump_kernel() to detect kdump kernel
  net/bnx2x: Use is_kdump_kernel() to detect kdump kernel

 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  | 3 ++-
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 3 ++-
 include/linux/mlx4/device.h                      | 3 ++-
 kernel/crash_dump.c                              | 1 +
 4 files changed, 7 insertions(+), 3 deletions(-)

-- 
1.8.3.4

[PATCH net-next 1/3] crash_dump: Make is_kdump_kernel() accessible from modules

[Amir Vadai]

In order to make is_kdump_kernel() accessible from modules, need to
make elfcorehdr_addr exported.
This was rejected in the past [1] because reset_devices was prefered in
that context (reseting the device in kdump kernel), but now there are
some network drivers that need to reduce memory usage when loaded from
a kdump kernel.  And in that context, is_kdump_kernel() suits better.

[1] - https://lkml.org/lkml/2011/1/27/341

CC: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 kernel/crash_dump.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/crash_dump.c b/kernel/crash_dump.c
index c766ee5..b64e238 100644
--- a/kernel/crash_dump.c
+++ b/kernel/crash_dump.c
@@ -18,6 +18,7 @@ unsigned long saved_max_pfn;
  * it under CONFIG_CRASH_DUMP and not CONFIG_PROC_VMCORE.
  */
 unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX;
+EXPORT_SYMBOL_GPL(elfcorehdr_addr);
 
 /*
  * stores the size of elf header of crash image
-- 
1.8.3.4

[PATCH net-next 2/3] net/mlx4: Use is_kdump_kernel() to detect kdump kernel

[Amir Vadai]

Use is_kdump_kernel() to detect kdump kernel, instead of reset_devices.

Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 include/linux/mlx4/device.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h
index e15b154..b96be83 100644
--- a/include/linux/mlx4/device.h
+++ b/include/linux/mlx4/device.h
@@ -38,6 +38,7 @@
 #include <linux/completion.h>
 #include <linux/radix-tree.h>
 #include <linux/cpu_rmap.h>
+#include <linux/crash_dump.h>
 
 #include <linux/atomic.h>
 
@@ -1258,7 +1259,7 @@ int mlx4_vf_set_enable_smi_admin(struct mlx4_dev *dev, int slave, int port,
 /* Returns true if running in low memory profile (kdump kernel) */
 static inline bool mlx4_low_memory_profile(void)
 {
-	return reset_devices;
+	return is_kdump_kernel();
 }
 
 #endif /* MLX4_DEVICE_H */
-- 
1.8.3.4

[PATCH net-next 3/3] net/bnx2x: Use is_kdump_kernel() to detect kdump kernel

[Amir Vadai]

Use is_kdump_kernel() to detect kdump kernel, instead of
reset_devices.

CC: Ariel Elior <ariel.elior@qlogic.com>
CC: Michal Schmidt <mschmidt@redhat.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  | 3 ++-
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index dca1236..9d83fd9 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -21,6 +21,7 @@
 #include <linux/if_vlan.h>
 #include <linux/interrupt.h>
 #include <linux/ip.h>
+#include <linux/crash_dump.h>
 #include <net/tcp.h>
 #include <net/ipv6.h>
 #include <net/ip6_checksum.h>
@@ -64,7 +65,7 @@ static int bnx2x_calc_num_queues(struct bnx2x *bp)
 	int nq = bnx2x_num_queues ? : netif_get_num_default_rss_queues();
 
 	/* Reduce memory usage in kdump environment by using only one queue */
-	if (reset_devices)
+	if (is_kdump_kernel())
 		nq = 1;
 
 	nq = clamp(nq, 1, BNX2X_MAX_QUEUES(bp));
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 3871ec4..ba5b1af 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -41,6 +41,7 @@
 #include <linux/ethtool.h>
 #include <linux/mii.h>
 #include <linux/if_vlan.h>
+#include <linux/crash_dump.h>
 #include <net/ip.h>
 #include <net/ipv6.h>
 #include <net/tcp.h>
@@ -11943,7 +11944,7 @@ static int bnx2x_init_bp(struct bnx2x *bp)
 	bp->disable_tpa = disable_tpa;
 	bp->disable_tpa |= IS_MF_STORAGE_SD(bp) || IS_MF_FCOE_AFEX(bp);
 	/* Reduce memory usage in kdump environment by disabling TPA */
-	bp->disable_tpa |= reset_devices;
+	bp->disable_tpa |= is_kdump_kernel();
 
 	/* Set TPA flags */
 	if (bp->disable_tpa) {
-- 
1.8.3.4

Re: [PATCH net-next 0/3] Make is_kdump_kernel() accessible from modules

[Andi Kleen]

Amir Vadai <amirv@mellanox.com> writes:

> Hi,
>
> I'm sending this patchset to net-next because it touches a commit [1] that was
> applied lately to net-next.

It would be better to have a generic function for "run with small
memory", that can be also triggered in other ways, e.g. a sysctl or
a boot opion, with kdump just another user.

There are low memory cases which are not kdump.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH net-next 0/3] Make is_kdump_kernel() accessible from modules

[Vivek Goyal]

On Sun, Jul 27, 2014 at 05:48:32PM -0700, Andi Kleen wrote:
> Amir Vadai <amirv@mellanox.com> writes:
> 
> > Hi,
> >
> > I'm sending this patchset to net-next because it touches a commit [1] that was
> > applied lately to net-next.
> 
> It would be better to have a generic function for "run with small
> memory", that can be also triggered in other ways, e.g. a sysctl or
> a boot opion, with kdump just another user.
> 
> There are low memory cases which are not kdump.

I am fine with a boot option to suggest low memory profile and kdump
scripts can be modified to automatically append that command line option
to second kernel.

Thanks
Vivek

Re: [PATCH net-next 0/3] Make is_kdump_kernel() accessible from modules

[David Miller]

From: Vivek Goyal <vgoyal@redhat.com>
Date: Mon, 28 Jul 2014 08:26:09 -0400

> On Sun, Jul 27, 2014 at 05:48:32PM -0700, Andi Kleen wrote:
>> Amir Vadai <amirv@mellanox.com> writes:
>> 
>> > Hi,
>> >
>> > I'm sending this patchset to net-next because it touches a commit [1] that was
>> > applied lately to net-next.
>> 
>> It would be better to have a generic function for "run with small
>> memory", that can be also triggered in other ways, e.g. a sysctl or
>> a boot opion, with kdump just another user.
>> 
>> There are low memory cases which are not kdump.
> 
> I am fine with a boot option to suggest low memory profile and kdump
> scripts can be modified to automatically append that command line option
> to second kernel.

I'm therefore going to assume that this patch series will be updated when the
new facility is added.

Re: [PATCH net-next 0/3] Make is_kdump_kernel() accessible from modules

[Vivek Goyal]

On Tue, Jul 29, 2014 at 04:46:32PM -0700, David Miller wrote:
> From: Vivek Goyal <vgoyal@redhat.com>
> Date: Mon, 28 Jul 2014 08:26:09 -0400
> 
> > On Sun, Jul 27, 2014 at 05:48:32PM -0700, Andi Kleen wrote:
> >> Amir Vadai <amirv@mellanox.com> writes:
> >> 
> >> > Hi,
> >> >
> >> > I'm sending this patchset to net-next because it touches a commit [1] that was
> >> > applied lately to net-next.
> >> 
> >> It would be better to have a generic function for "run with small
> >> memory", that can be also triggered in other ways, e.g. a sysctl or
> >> a boot opion, with kdump just another user.
> >> 
> >> There are low memory cases which are not kdump.
> > 
> > I am fine with a boot option to suggest low memory profile and kdump
> > scripts can be modified to automatically append that command line option
> > to second kernel.
> 
> I'm therefore going to assume that this patch series will be updated when the
> new facility is added.

Hi Amir,

Can you please add a new kernel parameter say, "low_memory_env" and change
rest of the patches to make use of this kernel parameter and repost.

I think for the drivers who were using "reset_devices" to switch to low
memory profile, you will have to retain that otherwise new kernel will
be broken with old kexec-tools/scripts which don't know that a new
parameter "low_memory_env" needs to be passed. 

May be we can do following.

/* TODO: Get rid of dependency on reset_devices after some time */
if (low_memory_env || reset_devices)
	use_low_memory_profile;

Thanks
Vivek

Re: [PATCH net-next 0/3] Make is_kdump_kernel() accessible from modules

[Amir Vadai]

On 7/30/2014 3:35 PM, Vivek Goyal wrote:
> On Tue, Jul 29, 2014 at 04:46:32PM -0700, David Miller wrote:
>> From: Vivek Goyal <vgoyal@redhat.com>
>> Date: Mon, 28 Jul 2014 08:26:09 -0400
>>
>>> On Sun, Jul 27, 2014 at 05:48:32PM -0700, Andi Kleen wrote:
>>>> Amir Vadai <amirv@mellanox.com> writes:
>>>>
>>>>> Hi,
>>>>>
>>>>> I'm sending this patchset to net-next because it touches a commit [1] that was
>>>>> applied lately to net-next.
>>>>
>>>> It would be better to have a generic function for "run with small
>>>> memory", that can be also triggered in other ways, e.g. a sysctl or
>>>> a boot opion, with kdump just another user.
>>>>
>>>> There are low memory cases which are not kdump.
>>>
>>> I am fine with a boot option to suggest low memory profile and kdump
>>> scripts can be modified to automatically append that command line option
>>> to second kernel.
>>
>> I'm therefore going to assume that this patch series will be updated when the
>> new facility is added.
> 
> Hi Amir,
> 
> Can you please add a new kernel parameter say, "low_memory_env" and change
> rest of the patches to make use of this kernel parameter and repost.
Sure.

I thought about it some more in the last day - aren't we making life too
complicated? I think the trigger that my driver (and maybe other) would
like to see is is_kdump_kernel and not is_low_mem - in mlx4_en we need
two assumptions that derive from is_kdump_kernel():
1. We are low on memory
2. This is kdump environment, and therefore we can assume that not many
vlans be used, no Infiniband support, performance can be compromised etc.
Just saying low memory might be less accurate - and will add some
complexity with the user space scripts (I know it is not very
complicated - but what are the benefits?)

If I didn't convince you :) - I have a small technical question:
I was looking for a place to put the __setup() and the low_mem_env
declaration and couldn't decide by myself - do you have a suggestion for
a proper c file and h file?

> 
> I think for the drivers who were using "reset_devices" to switch to low
> memory profile, you will have to retain that otherwise new kernel will
> be broken with old kexec-tools/scripts which don't know that a new
> parameter "low_memory_env" needs to be passed. 
> 
> May be we can do following.
> 
> /* TODO: Get rid of dependency on reset_devices after some time */
> if (low_memory_env || reset_devices)
> 	use_low_memory_profile;
Agree (This is part of the complexity I referred to above)

> 
> Thanks
> Vivek
> 

Amir

Re: [PATCH net-next 0/3] Make is_kdump_kernel() accessible from modules

[Vivek Goyal]

On Thu, Jul 31, 2014 at 09:51:53AM +0300, Amir Vadai wrote:
> On 7/30/2014 3:35 PM, Vivek Goyal wrote:
> > On Tue, Jul 29, 2014 at 04:46:32PM -0700, David Miller wrote:
> >> From: Vivek Goyal <vgoyal@redhat.com>
> >> Date: Mon, 28 Jul 2014 08:26:09 -0400
> >>
> >>> On Sun, Jul 27, 2014 at 05:48:32PM -0700, Andi Kleen wrote:
> >>>> Amir Vadai <amirv@mellanox.com> writes:
> >>>>
> >>>>> Hi,
> >>>>>
> >>>>> I'm sending this patchset to net-next because it touches a commit [1] that was
> >>>>> applied lately to net-next.
> >>>>
> >>>> It would be better to have a generic function for "run with small
> >>>> memory", that can be also triggered in other ways, e.g. a sysctl or
> >>>> a boot opion, with kdump just another user.
> >>>>
> >>>> There are low memory cases which are not kdump.
> >>>
> >>> I am fine with a boot option to suggest low memory profile and kdump
> >>> scripts can be modified to automatically append that command line option
> >>> to second kernel.
> >>
> >> I'm therefore going to assume that this patch series will be updated when the
> >> new facility is added.
> > 
> > Hi Amir,
> > 
> > Can you please add a new kernel parameter say, "low_memory_env" and change
> > rest of the patches to make use of this kernel parameter and repost.
> Sure.
> 
> I thought about it some more in the last day - aren't we making life too
> complicated? I think the trigger that my driver (and maybe other) would
> like to see is is_kdump_kernel and not is_low_mem - in mlx4_en we need
> two assumptions that derive from is_kdump_kernel():
> 1. We are low on memory
> 2. This is kdump environment, and therefore we can assume that not many
> vlans be used, no Infiniband support, performance can be compromised etc.
> Just saying low memory might be less accurate - and will add some
> complexity with the user space scripts (I know it is not very
> complicated - but what are the benefits?)

If there are performance implications of low memory profile, then user
needs to live with it. Otherwise they should not use low_memory_env
boot option to begin with.

> 
> If I didn't convince you :) - I have a small technical question:
> I was looking for a place to put the __setup() and the low_mem_env
> declaration and couldn't decide by myself - do you have a suggestion for
> a proper c file and h file?

As it is a boot option which will be applicable across arches, I think
this can be implemented along the lines of "reset_devices". Look at
init/main.c

I think a bigger problem is that low memory is a relative term. And
drivers writers will be thinking that what does low memory mean and
how much memory should they pre-allocate.

Is 256MB a low memory or will 2GB be considered a low memory. And
later there might be a discussion that a preallocation of memory which
suits 256MB is not the perfect fit for 2GB memory.

I am more concerned about it. By using is_kernel_kdump() we pretty
much made this profile custom for kdump requirements and other
usages can define their own boot options to meet their requirements.

Andi, what do you think about continuing with is_kernel_kdump() for
this case. Defining a new boot option low_memory_evn will work for
now but I got a feeling we will have conflicts later that what does
low memory mean and how that impacts the choice of profile.
IOW, low_memory_env sounds very generic and little vague.

Thanks
Vivek

Re: [PATCH net-next 0/3] Make is_kdump_kernel() accessible from modules

[David Miller]

From: Vivek Goyal <vgoyal@redhat.com>
Date: Thu, 31 Jul 2014 09:07:56 -0400

> If there are performance implications of low memory profile, then user
> needs to live with it. Otherwise they should not use low_memory_env
> boot option to begin with.

I personally think we should avoid changing scripts, by having a kdump
kernel force this low_mem profile to be true.

Re: [PATCH net-next 0/3] Make is_kdump_kernel() accessible from modules

[Vivek Goyal]

On Thu, Jul 31, 2014 at 12:22:44PM -0700, David Miller wrote:
> From: Vivek Goyal <vgoyal@redhat.com>
> Date: Thu, 31 Jul 2014 09:07:56 -0400
> 
> > If there are performance implications of low memory profile, then user
> > needs to live with it. Otherwise they should not use low_memory_env
> > boot option to begin with.
> 
> I personally think we should avoid changing scripts, by having a kdump
> kernel force this low_mem profile to be true.

If we go with current implementation, we don't have to change any of the
existing scripts.  Current implementation enforces low mem profile in
drivers for kdump kernel.

I am growing more skeptical of suggested low_mem_env boot option as there
will be contention that for different amount of low memory there should
be different profiles. One might argue that for 256MB use one kind of
profile while same profile might not be best fit for 2G of memory.

So instead of venturing into too generic option, I am fine with sticking
to a more custom option which works for kdump use case (is_kdump_kernel()).

So I would like to ack this patch series in current form.

Acked-by: Vivek Goyal <vgoyal@redhat.com>

Thanks
Vivek

Re: [PATCH net-next 0/3] Make is_kdump_kernel() accessible from modules

[Amir Vadai]

On 7/31/2014 10:22 PM, David Miller wrote:
> From: Vivek Goyal <vgoyal@redhat.com>
> Date: Thu, 31 Jul 2014 09:07:56 -0400
> 
>> If there are performance implications of low memory profile, then user
>> needs to live with it. Otherwise they should not use low_memory_env
>> boot option to begin with.
> 
> I personally think we should avoid changing scripts, by having a kdump
> kernel force this low_mem profile to be true.
Hi,

Is there work needed to be done on those patches? It seems that the
approach taken by those patches is accepted at the end. Also, Vivek
Goyal has acked it.

This patch is fixing stuff [1] that got into last 'net-next' (3.17).
Should I wait for it to appear in the next 'net' and send the patchset
to 'net' or can I send it now to 'net-next'?

[1] ea1c1af ("net/mlx4_en: Reduce memory consumption on kdump kernel")
    and
    2599d85 ("net/mlx4_core: Use low memory profile on kdump kernel")

Thanks,
Amir

> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>