| Message ID | 20211007175000.2334713-3-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mqprio fixup and simplify code. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Series has a cover letter |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 7 this patch: 7 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 22 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 7 this patch: 7 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
On Thu, 7 Oct 2021 19:49:58 +0200 Sebastian Andrzej Siewior wrote: > + __u64 bytes = 0; > + __u64 packets = 0; u64 is fine, no need to perpetuate the over-use of the user space types.
On Thu, Oct 7, 2021 at 10:51 AM Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > > Since day one __gnet_stats_copy_basic() always assigned the value to the > bstats argument overwriting the previous value. > > Based on review there are five users of that function as of today: > - est_fetch_counters(), ___gnet_stats_copy_basic() > memsets() bstats to zero, single invocation. > > - mq_dump(), mqprio_dump(), mqprio_dump_class_stats() > memsets() bstats to zero, multiple invocation but does not use the > function due to !qdisc_is_percpu_stats(). > > It will probably simplify in percpu stats case if the value would be > added and not just stored You at least need to rename it before doing so, otherwise "copy" would be too confusing.
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c index e491b083b3485..e12e544a7ab0f 100644 --- a/net/core/gen_stats.c +++ b/net/core/gen_stats.c @@ -143,6 +143,8 @@ __gnet_stats_copy_basic(const seqcount_t *running, struct gnet_stats_basic_packed *b) { unsigned int seq; + __u64 bytes = 0; + __u64 packets = 0; if (cpu) { __gnet_stats_copy_basic_cpu(bstats, cpu); @@ -151,9 +153,12 @@ __gnet_stats_copy_basic(const seqcount_t *running, do { if (running) seq = read_seqcount_begin(running); - bstats->bytes = b->bytes; - bstats->packets = b->packets; + bytes = b->bytes; + packets = b->packets; } while (running && read_seqcount_retry(running, seq)); + + bstats->bytes += bytes; + bstats->packets += packets; } EXPORT_SYMBOL(__gnet_stats_copy_basic);
Since day one __gnet_stats_copy_basic() always assigned the value to the bstats argument overwriting the previous value. Based on review there are five users of that function as of today: - est_fetch_counters(), ___gnet_stats_copy_basic() memsets() bstats to zero, single invocation. - mq_dump(), mqprio_dump(), mqprio_dump_class_stats() memsets() bstats to zero, multiple invocation but does not use the function due to !qdisc_is_percpu_stats(). It will probably simplify in percpu stats case if the value would be added and not just stored. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- net/core/gen_stats.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)