lib: qmem show changes (header and max) #2859

louberger · 2018-08-17T17:33:33Z

 add header to show qmem, align table with headers
 add tracking of max # allocs and max bytes alloc'ed

LabN-CI · 2018-08-17T18:03:24Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/2859 `43227f2`
Date	08/17/2018
Start	13:40:10
Finish	14:03:19
Run-Time	23:09
Total	1816
Pass	1816
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2018-08-17-13:40:10.txt
Log	autoscript-2018-08-17-13:40:55.log.bz2

For details, please contact louberger

NetDEF-CI · 2018-08-17T18:28:55Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-4890/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Checkout code: Successful with additional warnings:

Report for memory_vty.c | 2 issues
===============================================
< WARNING: line over 80 characters
< #105: FILE: /tmp/f1-23293/memory_vty.c:105:

CLANG Static Analyzer Summary

Github Pull Request 2859, comparing to Git base SHA e0909ff

No Changes in Static Analysis warnings compared to base

4 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-4890/artifact/shared/static_analysis/index.html

eqvinox

This won't work correctly under MT scenarios.

eqvinox · 2018-08-18T18:23:15Z

lib/memory.c

+	oldsize = 1 + atomic_fetch_add_explicit(&mt->n_alloc, 1,
+						memory_order_relaxed);
+
+	if (oldsize > atomic_load_explicit(&mt->n_max, memory_order_relaxed))


This is not how atomics work.

eqvinox · 2018-08-18T18:23:31Z

lib/memory.c

+	oldsize = mallocsz +
+		atomic_fetch_add_explicit(&mt->total, mallocsz,
+					  memory_order_relaxed);
+	if (oldsize > atomic_load_explicit(&mt->max_size, memory_order_relaxed))


Same here, this won't work correctly in MT.

louberger · 2018-08-19T00:01:28Z

Yes but it's a close enough approximation. Do you really want to lock this code?

…

----------

On August 18, 2018 1:25:04 PM David Lamparter ***@***.***> wrote: eqvinox requested changes on this pull request. This won't work correctly under MT scenarios. > @@ -38,7 +38,12 @@ static inline void mt_count_alloc(struct memtype *mt, > size_t size, void *ptr) { size_t oldsize; - atomic_fetch_add_explicit(&mt->n_alloc, 1, memory_order_relaxed); + oldsize = 1 + atomic_fetch_add_explicit(&mt->n_alloc, 1, + memory_order_relaxed); + + if (oldsize > atomic_load_explicit(&mt->n_max, memory_order_relaxed)) This is not how atomics work. > @@ -51,7 +56,12 @@ static inline void mt_count_alloc(struct memtype *mt, > size_t size, void *ptr) #ifdef HAVE_MALLOC_USABLE_SIZE size_t mallocsz = malloc_usable_size(ptr); - atomic_fetch_add_explicit(&mt->total, mallocsz, memory_order_relaxed); + oldsize = mallocsz + + atomic_fetch_add_explicit(&mt->total, mallocsz, + memory_order_relaxed); + if (oldsize > atomic_load_explicit(&mt->max_size, memory_order_relaxed)) Same here, this won't work correctly in MT. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #2859 (review)

eqvinox · 2018-08-20T16:23:09Z

Yes but it's a close enough approximation. Do you really want to lock this code?

Point of the atomics is to not lock ;) ...

We can track the max, but I would actually prefer tracking the total lifetime numbers (i.e. separately count allocations and frees, instead of having frees decrement the allocs.) Do you specifically want the max?

(max can be done in atomics too, albeit slightly more complicated)

louberger · 2018-08-20T17:41:28Z

----------

On August 20, 2018 11:23:40 AM David Lamparter ***@***.***> wrote: > Yes but it's a close enough approximation. Do you really want to lock this > code? Point of the atomics is not to lock ;) ... We can track the max, but I would actually prefer tracking the total lifetime numbers (i.e. separately count allocations and frees, instead of having frees decrement the allocs.)

I agree. I thought about changing it and decided I didn't care enough to make the change.

Do you specifically want the max?

Yes. Thanks the sole purpose of the change.

(max can be done in atomics too, albeit slightly more complicated)

I think an approximate value is good enough. I'd even be fine without using atomics on the max fields, but expected some would object. Please propose your preferred solution.... Thanks

…

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #2859 (comment)

eqvinox · 2018-08-20T19:10:43Z

@louberger alright gimme a bit to find my goat for the atomics gods.

eqvinox · 2018-08-23T16:02:32Z

I'm gonna close this for the moment, I'm working on it and will have a new PR up in a few days probably. We've lived without the numbers for years, we can survive a few days 😄

louberger · 2018-08-23T17:11:22Z

Closing someone else's PR is generally considered bad form...

…

----------

On August 23, 2018 11:03:06 AM David Lamparter ***@***.***> wrote: I'm gonna close this for the moment, I'm working on it and will have a new PR up in a few days probably. We've lived without the numbers for years, we can survive a few days :smile: -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #2859 (comment)

mjstapp

Added a reference to lib/thread.c where a different atomic pattern is being used for something similar

mjstapp · 2018-08-28T15:54:19Z

lib/memory.c

+	oldsize = 1 + atomic_fetch_add_explicit(&mt->n_alloc, 1,
+						memory_order_relaxed);
+
+	if (oldsize > atomic_load_explicit(&mt->n_max, memory_order_relaxed))


we've got some code in lib/thread.c that's using atomic_compare_exchange_weak_explicit() in a loop to accomplish this sort of thing - maybe that pattern could be reused?

louberger · 2018-08-28T18:11:50Z

Yeah, that's what I was trying to avoid as it locks the critical code:

…

__sync_synchronize(); \ typeof(*atom) rval = \ __sync_val_compare_and_swap(_atom, *_expect, _desire); \ __sync_synchronize(); Sort of like your volatile argument -- the current approach balances precision and cost. Thanks for the thought!

On 8/28/2018 11:58 AM, Mark Stapp wrote: ***@***.**** commented on this pull request. Added a reference to lib/thread.c where a different atomic pattern is being used for something similar ------------------------------------------------------------------------ In lib/memory.c <#2859 (comment)>: > @@ -38,7 +38,12 @@ static inline void mt_count_alloc(struct memtype *mt, size_t size, void *ptr) { size_t oldsize; - atomic_fetch_add_explicit(&mt->n_alloc, 1, memory_order_relaxed); + oldsize = 1 + atomic_fetch_add_explicit(&mt->n_alloc, 1, + memory_order_relaxed); + + if (oldsize > atomic_load_explicit(&mt->n_max, memory_order_relaxed)) we've got some code in lib/thread.c that's using |atomic_compare_exchange_weak_explicit()| in a loop to accomplish this sort of thing - maybe that pattern could be reused? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2859 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGRWyNUgiFey3K7v7Dv4Eaogmhf1GNdnks5uVWiqgaJpZM4WB7fd>.

mjstapp · 2018-08-28T18:58:00Z

hmm - that's only for older compilers, isn't it - and ... it's not really a lock - it's just a barrier? I mean, in order to support MT we have to take some precautions, even though there's sometimes a cost involved.

louberger · 2018-08-28T21:00:14Z

@eqvinox @mjstapp okay revised with suggested changes and additions. @donaldsharp Note that the result is an approximation in the MT case -- I'm intentionally not testing the result of atomic_compare_exchange_weak_explicit which would fully cover the MT case, but the while loop required is overly expensive IMO. I'll fully cover the MT case if others think this is critical.

LabN-CI · 2018-08-28T21:31:58Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/2859 `6497310`
Date	08/28/2018
Start	17:08:42
Finish	17:31:52
Run-Time	23:10
Total	1816
Pass	1816
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2018-08-28-17:08:42.txt
Log	autoscript-2018-08-28-17:09:26.log.bz2

For details, please contact louberger

add header to show qmem, align table with headers add tracking of max # allocs and max bytes alloc'ed Signed-off-by: Lou Berger <lberger@labn.net>

Signed-off-by: Lou Berger <lberger@labn.net>

LabN-CI · 2018-08-28T23:54:03Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/2859 `7a15333`
Date	08/28/2018
Start	19:30:19
Finish	19:53:58
Run-Time	23:39
Total	1816
Pass	1816
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2018-08-28-19:30:19.txt
Log	autoscript-2018-08-28-19:31:20.log.bz2

For details, please contact louberger

NetDEF-CI · 2018-08-29T00:17:56Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5078/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

CLANG Static Analyzer Summary

Github Pull Request 2859, comparing to Git base SHA 96487ee

No Changes in Static Analysis warnings compared to base

4 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5078/artifact/shared/static_analysis/index.html

eqvinox requested changes Aug 18, 2018

View reviewed changes

eqvinox closed this Aug 23, 2018

louberger reopened this Aug 26, 2018

mjstapp reviewed Aug 28, 2018

View reviewed changes

louberger force-pushed the working/master/meminfo branch from 43227f2 to 6497310 Compare August 28, 2018 20:56

This comment has been minimized.

Sign in to view

louberger mentioned this pull request Aug 28, 2018

atomic_add_fetch_explicit doesn't compile on all platforms #2936

Closed

louberger added 2 commits August 28, 2018 19:22

lib: qmem show changes (header and max)

13f1ba4

add header to show qmem, align table with headers add tracking of max # allocs and max bytes alloc'ed Signed-off-by: Lou Berger <lberger@labn.net>

lib: atomic_add_fetch_explicit not supported on all platforms

7a15333

Signed-off-by: Lou Berger <lberger@labn.net>

louberger force-pushed the working/master/meminfo branch from 6497310 to 7a15333 Compare August 28, 2018 23:22

eqvinox merged commit 66a9aa8 into FRRouting:master Aug 30, 2018

louberger deleted the working/master/meminfo branch March 17, 2020 11:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: qmem show changes (header and max) #2859

lib: qmem show changes (header and max) #2859

louberger commented Aug 17, 2018

LabN-CI commented Aug 17, 2018

NetDEF-CI commented Aug 17, 2018

eqvinox left a comment

eqvinox Aug 18, 2018

eqvinox Aug 18, 2018

louberger commented Aug 19, 2018 via email

eqvinox commented Aug 20, 2018 •

edited

Loading

louberger commented Aug 20, 2018 via email

eqvinox commented Aug 20, 2018

eqvinox commented Aug 23, 2018

louberger commented Aug 23, 2018 via email

mjstapp left a comment

mjstapp Aug 28, 2018

louberger commented Aug 28, 2018 via email

mjstapp commented Aug 28, 2018

louberger commented Aug 28, 2018

This comment has been minimized.

LabN-CI commented Aug 28, 2018

LabN-CI commented Aug 28, 2018

NetDEF-CI commented Aug 29, 2018

lib: qmem show changes (header and max) #2859

lib: qmem show changes (header and max) #2859

Conversation

louberger commented Aug 17, 2018

LabN-CI commented Aug 17, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Aug 17, 2018

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

Checkout code: Successful with additional warnings:

CLANG Static Analyzer Summary

No Changes in Static Analysis warnings compared to base

4 Static Analyzer issues remaining.

eqvinox left a comment

Choose a reason for hiding this comment

eqvinox Aug 18, 2018

Choose a reason for hiding this comment

eqvinox Aug 18, 2018

Choose a reason for hiding this comment

louberger commented Aug 19, 2018 via email

eqvinox commented Aug 20, 2018 • edited Loading

louberger commented Aug 20, 2018 via email

eqvinox commented Aug 20, 2018

eqvinox commented Aug 23, 2018

louberger commented Aug 23, 2018 via email

mjstapp left a comment

Choose a reason for hiding this comment

mjstapp Aug 28, 2018

Choose a reason for hiding this comment

louberger commented Aug 28, 2018 via email

mjstapp commented Aug 28, 2018

louberger commented Aug 28, 2018

This comment has been minimized.

LabN-CI commented Aug 28, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

LabN-CI commented Aug 28, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Aug 29, 2018

Continuous Integration Result: SUCCESSFUL

CLANG Static Analyzer Summary

No Changes in Static Analysis warnings compared to base

4 Static Analyzer issues remaining.

eqvinox commented Aug 20, 2018 •

edited

Loading