[PATCH 0/3] hw/net,virtfs-proxy-helper: Reduce .data footprint

[PATCH 0/3] hw/net,virtfs-proxy-helper: Reduce .data footprint

[Philippe Mathieu-Daudé]

More memory footprint reduction, similar to:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg00984.html

The elf-dissector tool [1] [2] helped to notice the big array.

[1] https://phabricator.kde.org/source/elf-dissector/
[2] https://www.volkerkrause.eu/2019/06/22/elf-dissector-aarch64-support.html

Philippe Mathieu-Daudé (3):
  hw/net/e1000: Add readops/writeops typedefs
  hw/net/e1000: Move macreg[] arrays to .rodata to save 1MiB of .data
  virtfs-proxy-helper: Make the helper_opts[] array const

 fsdev/virtfs-proxy-helper.c | 2 +-
 hw/net/e1000.c              | 6 ++++--
 hw/net/e1000e_core.c        | 6 ++++--
 3 files changed, 9 insertions(+), 5 deletions(-)

-- 
2.21.1

[PATCH 1/3] hw/net/e1000: Add readops/writeops typedefs

[Philippe Mathieu-Daudé]

Express the macreg[] arrays using typedefs.
No logical changes introduced here.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/e1000.c       | 6 ++++--
 hw/net/e1000e_core.c | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 0b833d5a15..972d9b5083 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1150,7 +1150,8 @@ set_ims(E1000State *s, int index, uint32_t val)
 }
 
 #define getreg(x)    [x] = mac_readreg
-static uint32_t (*macreg_readops[])(E1000State *, int) = {
+typedef uint32_t (*readops)(E1000State *, int);
+static readops macreg_readops[] = {
     getreg(PBA),      getreg(RCTL),     getreg(TDH),      getreg(TXDCTL),
     getreg(WUFC),     getreg(TDT),      getreg(CTRL),     getreg(LEDCTL),
     getreg(MANC),     getreg(MDIC),     getreg(SWSM),     getreg(STATUS),
@@ -1205,7 +1206,8 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
 enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
 
 #define putreg(x)    [x] = mac_writereg
-static void (*macreg_writeops[])(E1000State *, int, uint32_t) = {
+typedef void (*writeops)(E1000State *, int, uint32_t);
+static writeops macreg_writeops[] = {
     putreg(PBA),      putreg(EERD),     putreg(SWSM),     putreg(WUFC),
     putreg(TDBAL),    putreg(TDBAH),    putreg(TXDCTL),   putreg(RDBAH),
     putreg(RDBAL),    putreg(LEDCTL),   putreg(VET),      putreg(FCRUC),
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index 94ea34dca5..38bdb90114 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2855,7 +2855,8 @@ e1000e_set_gcr(E1000ECore *core, int index, uint32_t val)
 }
 
 #define e1000e_getreg(x)    [x] = e1000e_mac_readreg
-static uint32_t (*e1000e_macreg_readops[])(E1000ECore *, int) = {
+typedef uint32_t (*readops)(E1000ECore *, int);
+static readops e1000e_macreg_readops[] = {
     e1000e_getreg(PBA),
     e1000e_getreg(WUFC),
     e1000e_getreg(MANC),
@@ -3061,7 +3062,8 @@ static uint32_t (*e1000e_macreg_readops[])(E1000ECore *, int) = {
 enum { E1000E_NREADOPS = ARRAY_SIZE(e1000e_macreg_readops) };
 
 #define e1000e_putreg(x)    [x] = e1000e_mac_writereg
-static void (*e1000e_macreg_writeops[])(E1000ECore *, int, uint32_t) = {
+typedef void (*writeops)(E1000ECore *, int, uint32_t);
+static writeops e1000e_macreg_writeops[] = {
     e1000e_putreg(PBA),
     e1000e_putreg(SWSM),
     e1000e_putreg(WUFC),
-- 
2.21.1

Re: [PATCH 1/3] hw/net/e1000: Add readops/writeops typedefs

[Dmitry Fleytman]

> On 5 Mar 2020, at 3:04, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
> Express the macreg[] arrays using typedefs.
> No logical changes introduced here.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>


Reviewed-by: Dmitry Fleytman <dmitry.fleytman@gmail.com>


> ---
> hw/net/e1000.c       | 6 ++++--
> hw/net/e1000e_core.c | 6 ++++--
> 2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 0b833d5a15..972d9b5083 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -1150,7 +1150,8 @@ set_ims(E1000State *s, int index, uint32_t val)
> }
> 
> #define getreg(x)    [x] = mac_readreg
> -static uint32_t (*macreg_readops[])(E1000State *, int) = {
> +typedef uint32_t (*readops)(E1000State *, int);
> +static readops macreg_readops[] = {
>     getreg(PBA),      getreg(RCTL),     getreg(TDH),      getreg(TXDCTL),
>     getreg(WUFC),     getreg(TDT),      getreg(CTRL),     getreg(LEDCTL),
>     getreg(MANC),     getreg(MDIC),     getreg(SWSM),     getreg(STATUS),
> @@ -1205,7 +1206,8 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
> enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
> 
> #define putreg(x)    [x] = mac_writereg
> -static void (*macreg_writeops[])(E1000State *, int, uint32_t) = {
> +typedef void (*writeops)(E1000State *, int, uint32_t);
> +static writeops macreg_writeops[] = {
>     putreg(PBA),      putreg(EERD),     putreg(SWSM),     putreg(WUFC),
>     putreg(TDBAL),    putreg(TDBAH),    putreg(TXDCTL),   putreg(RDBAH),
>     putreg(RDBAL),    putreg(LEDCTL),   putreg(VET),      putreg(FCRUC),
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index 94ea34dca5..38bdb90114 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -2855,7 +2855,8 @@ e1000e_set_gcr(E1000ECore *core, int index, uint32_t val)
> }
> 
> #define e1000e_getreg(x)    [x] = e1000e_mac_readreg
> -static uint32_t (*e1000e_macreg_readops[])(E1000ECore *, int) = {
> +typedef uint32_t (*readops)(E1000ECore *, int);
> +static readops e1000e_macreg_readops[] = {
>     e1000e_getreg(PBA),
>     e1000e_getreg(WUFC),
>     e1000e_getreg(MANC),
> @@ -3061,7 +3062,8 @@ static uint32_t (*e1000e_macreg_readops[])(E1000ECore *, int) = {
> enum { E1000E_NREADOPS = ARRAY_SIZE(e1000e_macreg_readops) };
> 
> #define e1000e_putreg(x)    [x] = e1000e_mac_writereg
> -static void (*e1000e_macreg_writeops[])(E1000ECore *, int, uint32_t) = {
> +typedef void (*writeops)(E1000ECore *, int, uint32_t);
> +static writeops e1000e_macreg_writeops[] = {
>     e1000e_putreg(PBA),
>     e1000e_putreg(SWSM),
>     e1000e_putreg(WUFC),
> -- 
> 2.21.1
> 

[PATCH 2/3] hw/net/e1000: Move macreg[] arrays to .rodata to save 1MiB of .data

[Philippe Mathieu-Daudé]

Each array consumes 256KiB of .data. As we do not reassign entries,
we can move it to the .rodata section, and save a total of 1MiB of
.data (size reported on x86_64 host).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/e1000.c       | 4 ++--
 hw/net/e1000e_core.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 972d9b5083..9233248c9a 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1151,7 +1151,7 @@ set_ims(E1000State *s, int index, uint32_t val)
 
 #define getreg(x)    [x] = mac_readreg
 typedef uint32_t (*readops)(E1000State *, int);
-static readops macreg_readops[] = {
+static const readops macreg_readops[] = {
     getreg(PBA),      getreg(RCTL),     getreg(TDH),      getreg(TXDCTL),
     getreg(WUFC),     getreg(TDT),      getreg(CTRL),     getreg(LEDCTL),
     getreg(MANC),     getreg(MDIC),     getreg(SWSM),     getreg(STATUS),
@@ -1207,7 +1207,7 @@ enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
 
 #define putreg(x)    [x] = mac_writereg
 typedef void (*writeops)(E1000State *, int, uint32_t);
-static writeops macreg_writeops[] = {
+static const writeops macreg_writeops[] = {
     putreg(PBA),      putreg(EERD),     putreg(SWSM),     putreg(WUFC),
     putreg(TDBAL),    putreg(TDBAH),    putreg(TXDCTL),   putreg(RDBAH),
     putreg(RDBAL),    putreg(LEDCTL),   putreg(VET),      putreg(FCRUC),
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index 38bdb90114..df957e0c1a 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2856,7 +2856,7 @@ e1000e_set_gcr(E1000ECore *core, int index, uint32_t val)
 
 #define e1000e_getreg(x)    [x] = e1000e_mac_readreg
 typedef uint32_t (*readops)(E1000ECore *, int);
-static readops e1000e_macreg_readops[] = {
+static const readops e1000e_macreg_readops[] = {
     e1000e_getreg(PBA),
     e1000e_getreg(WUFC),
     e1000e_getreg(MANC),
@@ -3063,7 +3063,7 @@ enum { E1000E_NREADOPS = ARRAY_SIZE(e1000e_macreg_readops) };
 
 #define e1000e_putreg(x)    [x] = e1000e_mac_writereg
 typedef void (*writeops)(E1000ECore *, int, uint32_t);
-static writeops e1000e_macreg_writeops[] = {
+static const writeops e1000e_macreg_writeops[] = {
     e1000e_putreg(PBA),
     e1000e_putreg(SWSM),
     e1000e_putreg(WUFC),
-- 
2.21.1

Re: [PATCH 2/3] hw/net/e1000: Move macreg[] arrays to .rodata to save 1MiB of .data

[Dmitry Fleytman]

> On 5 Mar 2020, at 3:04, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
> Each array consumes 256KiB of .data. As we do not reassign entries,
> we can move it to the .rodata section, and save a total of 1MiB of
> .data (size reported on x86_64 host).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>


Reviewed-by: Dmitry Fleytman <dmitry.fleytman@gmail.com>


> ---
> hw/net/e1000.c       | 4 ++--
> hw/net/e1000e_core.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 972d9b5083..9233248c9a 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -1151,7 +1151,7 @@ set_ims(E1000State *s, int index, uint32_t val)
> 
> #define getreg(x)    [x] = mac_readreg
> typedef uint32_t (*readops)(E1000State *, int);
> -static readops macreg_readops[] = {
> +static const readops macreg_readops[] = {
>     getreg(PBA),      getreg(RCTL),     getreg(TDH),      getreg(TXDCTL),
>     getreg(WUFC),     getreg(TDT),      getreg(CTRL),     getreg(LEDCTL),
>     getreg(MANC),     getreg(MDIC),     getreg(SWSM),     getreg(STATUS),
> @@ -1207,7 +1207,7 @@ enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
> 
> #define putreg(x)    [x] = mac_writereg
> typedef void (*writeops)(E1000State *, int, uint32_t);
> -static writeops macreg_writeops[] = {
> +static const writeops macreg_writeops[] = {
>     putreg(PBA),      putreg(EERD),     putreg(SWSM),     putreg(WUFC),
>     putreg(TDBAL),    putreg(TDBAH),    putreg(TXDCTL),   putreg(RDBAH),
>     putreg(RDBAL),    putreg(LEDCTL),   putreg(VET),      putreg(FCRUC),
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index 38bdb90114..df957e0c1a 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -2856,7 +2856,7 @@ e1000e_set_gcr(E1000ECore *core, int index, uint32_t val)
> 
> #define e1000e_getreg(x)    [x] = e1000e_mac_readreg
> typedef uint32_t (*readops)(E1000ECore *, int);
> -static readops e1000e_macreg_readops[] = {
> +static const readops e1000e_macreg_readops[] = {
>     e1000e_getreg(PBA),
>     e1000e_getreg(WUFC),
>     e1000e_getreg(MANC),
> @@ -3063,7 +3063,7 @@ enum { E1000E_NREADOPS = ARRAY_SIZE(e1000e_macreg_readops) };
> 
> #define e1000e_putreg(x)    [x] = e1000e_mac_writereg
> typedef void (*writeops)(E1000ECore *, int, uint32_t);
> -static writeops e1000e_macreg_writeops[] = {
> +static const writeops e1000e_macreg_writeops[] = {
>     e1000e_putreg(PBA),
>     e1000e_putreg(SWSM),
>     e1000e_putreg(WUFC),
> -- 
> 2.21.1
> 

[PATCH 3/3] virtfs-proxy-helper: Make the helper_opts[] array const

[Philippe Mathieu-Daudé]

Reduce a bit the memory footprint by making the helper_opts[]
array const.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 fsdev/virtfs-proxy-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index aa1ab2590d..de061a8a0e 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -43,7 +43,7 @@
 #define BTRFS_SUPER_MAGIC 0x9123683E
 #endif
 
-static struct option helper_opts[] = {
+static const struct option helper_opts[] = {
     {"fd", required_argument, NULL, 'f'},
     {"path", required_argument, NULL, 'p'},
     {"nodaemon", no_argument, NULL, 'n'},
-- 
2.21.1

Re: [PATCH 3/3] virtfs-proxy-helper: Make the helper_opts[] array const

[Greg Kurz]

On Thu,  5 Mar 2020 02:04:46 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Reduce a bit the memory footprint by making the helper_opts[]
> array const.
> 

... and, maybe most importantly, make it clear helper_opts[] is
not supposed to be modified at runtime.

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

Assuming this goes to the trivial tree:

Acked-by: Greg Kurz <groug@kaod.org>

>  fsdev/virtfs-proxy-helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index aa1ab2590d..de061a8a0e 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -43,7 +43,7 @@
>  #define BTRFS_SUPER_MAGIC 0x9123683E
>  #endif
>  
> -static struct option helper_opts[] = {
> +static const struct option helper_opts[] = {
>      {"fd", required_argument, NULL, 'f'},
>      {"path", required_argument, NULL, 'p'},
>      {"nodaemon", no_argument, NULL, 'n'},

Re: [PATCH 0/3] hw/net,virtfs-proxy-helper: Reduce .data footprint

[Stefano Garzarella]

On Thu, Mar 05, 2020 at 02:04:43AM +0100, Philippe Mathieu-Daudé wrote:
> More memory footprint reduction, similar to:
> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg00984.html
> 
> The elf-dissector tool [1] [2] helped to notice the big array.
> 
> [1] https://phabricator.kde.org/source/elf-dissector/
> [2] https://www.volkerkrause.eu/2019/06/22/elf-dissector-aarch64-support.html
> 

Thanks to share these links!

> Philippe Mathieu-Daudé (3):
>   hw/net/e1000: Add readops/writeops typedefs
>   hw/net/e1000: Move macreg[] arrays to .rodata to save 1MiB of .data
>   virtfs-proxy-helper: Make the helper_opts[] array const
> 
>  fsdev/virtfs-proxy-helper.c | 2 +-
>  hw/net/e1000.c              | 6 ++++--
>  hw/net/e1000e_core.c        | 6 ++++--
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 

Cool and clear changes!

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

Re: [PATCH 0/3] hw/net,virtfs-proxy-helper: Reduce .data footprint

[Philippe Mathieu-Daudé]

On 3/5/20 10:53 AM, Stefano Garzarella wrote:
> On Thu, Mar 05, 2020 at 02:04:43AM +0100, Philippe Mathieu-Daudé wrote:
>> More memory footprint reduction, similar to:
>> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg00984.html
>>
>> The elf-dissector tool [1] [2] helped to notice the big array.
>>
>> [1] https://phabricator.kde.org/source/elf-dissector/
>> [2] https://www.volkerkrause.eu/2019/06/22/elf-dissector-aarch64-support.html
>>
> 
> Thanks to share these links!

FYI the heap equivalent (besides that more powerful) is:
https://github.com/KDE/heaptrack

> 
>> Philippe Mathieu-Daudé (3):
>>    hw/net/e1000: Add readops/writeops typedefs
>>    hw/net/e1000: Move macreg[] arrays to .rodata to save 1MiB of .data
>>    virtfs-proxy-helper: Make the helper_opts[] array const
>>
>>   fsdev/virtfs-proxy-helper.c | 2 +-
>>   hw/net/e1000.c              | 6 ++++--
>>   hw/net/e1000e_core.c        | 6 ++++--
>>   3 files changed, 9 insertions(+), 5 deletions(-)
>>
> 
> Cool and clear changes!
> 
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> 
> Thanks,
> Stefano
> 

Re: [PATCH 0/3] hw/net,virtfs-proxy-helper: Reduce .data footprint

[Stefano Garzarella]

On Thu, Mar 05, 2020 at 11:37:22AM +0100, Philippe Mathieu-Daudé wrote:
> On 3/5/20 10:53 AM, Stefano Garzarella wrote:
> > On Thu, Mar 05, 2020 at 02:04:43AM +0100, Philippe Mathieu-Daudé wrote:
> > > More memory footprint reduction, similar to:
> > > https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg00984.html
> > > 
> > > The elf-dissector tool [1] [2] helped to notice the big array.
> > > 
> > > [1] https://phabricator.kde.org/source/elf-dissector/
> > > [2] https://www.volkerkrause.eu/2019/06/22/elf-dissector-aarch64-support.html
> > > 
> > 
> > Thanks to share these links!
> 
> FYI the heap equivalent (besides that more powerful) is:
> https://github.com/KDE/heaptrack

Cool, thanks!

Previously I used valgrind's massif tool to track the heap, but I'll try
heaptrack.

Cheers,
Stefano

Re: [PATCH 0/3] hw/net,virtfs-proxy-helper: Reduce .data footprint

[Paolo Bonzini]

On 05/03/20 02:04, Philippe Mathieu-Daudé wrote:
> More memory footprint reduction, similar to:
> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg00984.html
> 
> The elf-dissector tool [1] [2] helped to notice the big array.
> 
> [1] https://phabricator.kde.org/source/elf-dissector/
> [2] https://www.volkerkrause.eu/2019/06/22/elf-dissector-aarch64-support.html
> 
> Philippe Mathieu-Daudé (3):
>   hw/net/e1000: Add readops/writeops typedefs
>   hw/net/e1000: Move macreg[] arrays to .rodata to save 1MiB of .data
>   virtfs-proxy-helper: Make the helper_opts[] array const
> 
>  fsdev/virtfs-proxy-helper.c | 2 +-
>  hw/net/e1000.c              | 6 ++++--
>  hw/net/e1000e_core.c        | 6 ++++--
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

These can certainly go through qemu-trivial.

Paolo

Re: [PATCH 0/3] hw/net,virtfs-proxy-helper: Reduce .data footprint

[Laurent Vivier]

Le 06/03/2020 à 10:25, Paolo Bonzini a écrit :
> On 05/03/20 02:04, Philippe Mathieu-Daudé wrote:
>> More memory footprint reduction, similar to:
>> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg00984.html
>>
>> The elf-dissector tool [1] [2] helped to notice the big array.
>>
>> [1] https://phabricator.kde.org/source/elf-dissector/
>> [2] https://www.volkerkrause.eu/2019/06/22/elf-dissector-aarch64-support.html
>>
>> Philippe Mathieu-Daudé (3):
>>   hw/net/e1000: Add readops/writeops typedefs
>>   hw/net/e1000: Move macreg[] arrays to .rodata to save 1MiB of .data
>>   virtfs-proxy-helper: Make the helper_opts[] array const
>>
>>  fsdev/virtfs-proxy-helper.c | 2 +-
>>  hw/net/e1000.c              | 6 ++++--
>>  hw/net/e1000e_core.c        | 6 ++++--
>>  3 files changed, 9 insertions(+), 5 deletions(-)
>>
> 
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> These can certainly go through qemu-trivial.

Applied to my trivial-patches branch.

Thanks,
Laurent