Martin Pieuchot <m...@openbsd.org> writes:
> On 30/01/17(Mon) 22:14, Jeremie Courreges-Anglas wrote:
>> Very lightly tested. I used the existing percpu counters as examples.
>
> I like it.
>
>> I don't like the hardcoding of "32" in ip6_input() but I am not sure how
>> to solve it nicely; the easiest way would be to just kill those
>> ipv6-specific mbuf stats. Would anyone miss them?
>
> I believe it's fine to kill it.
Let's do that.
>> Thoughts?
>
> Some comments below.
>
>> Other netinet6/* counters will follow.
>
> \o/
>
>> @@ -514,14 +514,14 @@ frag6_input(struct mbuf **mp, int *offp,
>> m_freem(IP6_REASS_MBUF(af6));
>> free(af6, M_FTABLE, sizeof(*af6));
>> }
>> - ip6stat.ip6s_fragdropped += q6->ip6q_nfrag;
>> + counters_add(ip6counters, ip6s_fragdropped, q6->ip6q_nfrag);
>
> What about adding & using ip6stat_add()?
Alright.
>> Index: netinet6/icmp6.c
>> ===================================================================
>> RCS file: /d/cvs/src/sys/netinet6/icmp6.c,v
>> retrieving revision 1.197
>> diff -u -p -r1.197 icmp6.c
>> --- netinet6/icmp6.c 19 Jan 2017 14:49:19 -0000 1.197
>> +++ netinet6/icmp6.c 30 Jan 2017 21:02:30 -0000
>> @@ -1124,8 +1124,13 @@ icmp6_rip6_input(struct mbuf **mp, int o
>> } else
>> sorwakeup(last->inp_socket);
>> } else {
>> + struct counters_ref ref;
>> + uint64_t *counters;
>> +
>> m_freem(m);
>> - ip6stat.ip6s_delivered--;
>> + counters = counters_enter(&ref, ip6counters);
>> + counters[ip6s_delivered]--;
>> + counters_leave(&ref, ip6counters);
>
> Looks like you found a use case for introducing counters_dec().
Yes, but I don't know which threshold warrants introducing a new
function. So far those are the first cases for counters_dec(); that
includes wip rip6stat, icmp6stat, icmpstat and tcpstat. And I'm not
even sure those decrements make sense. I guess we can add counters_dec()
and then later remove it if it is deemed useless.
>> Index: netinet6/ip6_input.c
>> ===================================================================
>> RCS file: /d/cvs/src/sys/netinet6/ip6_input.c,v
>> retrieving revision 1.175
>> diff -u -p -r1.175 ip6_input.c
>> --- netinet6/ip6_input.c 29 Jan 2017 19:58:47 -0000 1.175
>> +++ netinet6/ip6_input.c 30 Jan 2017 21:02:30 -0000
>> @@ -117,7 +117,9 @@
>> struct in6_ifaddrhead in6_ifaddr;
>> struct niqueue ip6intrq = NIQUEUE_INITIALIZER(IFQ_MAXLEN, NETISR_IPV6);
>>
>> -struct ip6stat ip6stat;
>> +struct cpumem *ip6counters;
>> +
>> +int ip6_sysctl_ip6stat(void *, size_t *, void *);
>>
>> int ip6_check_rh0hdr(struct mbuf *, int *);
>>
>> @@ -158,6 +160,8 @@ ip6_init(void)
>> frag6_init();
>>
>> mq_init(&ip6send_mq, 64, IPL_SOFTNET);
>> +
>> + ip6counters = counters_alloc(ip6s_ncounters, M_COUNTERS);
>
> Could you do me a favor and kill the last argument of counters_alloc(9)?
Agreed.
>> Index: netinet6/raw_ip6.c
>> ===================================================================
>> RCS file: /d/cvs/src/sys/netinet6/raw_ip6.c,v
>> retrieving revision 1.103
>> diff -u -p -r1.103 raw_ip6.c
>> --- netinet6/raw_ip6.c 23 Jan 2017 16:31:24 -0000 1.103
>> +++ netinet6/raw_ip6.c 30 Jan 2017 21:02:30 -0000
>> @@ -211,6 +211,9 @@ rip6_input(struct mbuf **mp, int *offp,
>> } else
>> sorwakeup(last->inp_socket);
>> } else {
>> + struct counters_ref ref;
>> + uint64_t *counters;
>> +
>> rip6stat.rip6s_nosock++;
>> if (m->m_flags & M_MCAST)
>> rip6stat.rip6s_nosockmcast++;
>> @@ -222,7 +225,9 @@ rip6_input(struct mbuf **mp, int *offp,
>> ICMP6_PARAMPROB_NEXTHEADER,
>> prvnxtp - mtod(m, u_int8_t *));
>> }
>> - ip6stat.ip6s_delivered--;
>> + counters = counters_enter(&ref, ip6counters);
>> + counters[ip6s_delivered]--;
>> + counters_leave(&ref, ip6counters);
>
> Should we also use your counters_dec() here or do you think it's better
> to merge icmp6_rip6_input() and rip6_input()?
I'm fine with using counters_dec(), I can see that those functions are
almost twins but I don't want to open a can of worms...
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
