net: introduce IPv6 netstats#5178
Conversation
OlegHahm
added
Type: enhancement
OlegHahm
removed
the
State: waiting for other PR
| * @return A @ref netstats_t pointer to the statistics. | ||
| * @return NULL if no statistics are available. | ||
| */ | ||
| netstats_t *gnrc_ipv6_netif_get_stats(kernel_pid_t pid); |
There was a problem hiding this comment.
Wouldn't it be more suitable to put this into some unit file of the netstats module itself? Then you can also drop the @note section and it is immediately apparent, that you need said module compiled into the binary.
There was a problem hiding this comment.
IMO that would be conceptionally unclean. Actually, I think the better idea would be drop the netstats module completely and move its "functionality" into the shell.
There was a problem hiding this comment.
What do you think about the new solution?
There was a problem hiding this comment.
...and move its "functionality" into the shell.
It depends: Can I still use netstats then if I do not compile the shell, but send the stats over e.g. UDP to a server?
There was a problem hiding this comment.
Sure. The only thing in the netstats module was the _to_string() function that moved to the shell.
OlegHahm
added
the
CI: needs squashing
| } | ||
| #ifdef MODULE_NETSTATS_IPV6 | ||
| if_entry->stats.tx_success++; | ||
| if_entry->stats.tx_bytes += gnrc_pkt_len(pkt->next); |
There was a problem hiding this comment.
not sure if it's available here, but why not use the length field of the ipv6 header? Isn't that more accurate than gnrc_pkt_len(pkt->next)?
There was a problem hiding this comment.
IPv6 header length + ipv6->len; that would be. pkt->next may contain the netif header that isn't really data that is send over the interface.
There was a problem hiding this comment.
I agree that IPv6 header length + ipv6->len sounds like a good idea, but in which case can pkt->next contain a netif header and which type would pkt be in this case?
There was a problem hiding this comment.
Hm, looking at the code again, I wonder if the current solution is not more reasonable: we add the amount of data that is actually passed to the lower layer here.
There was a problem hiding this comment.
But this amount of data can also include padding, if I am not mistaken? AFAIK, the pkts in the pktbuf need to be size aligned(?)
There was a problem hiding this comment.
Hm, who does actually remove the padding before sending?
There was a problem hiding this comment.
Hm, no idea. In gnrc_pktbuf_static.c:423 the size for a pktnsip gets aligned, but I have no idea where the magic happens that gnrc_pkt_len reports the correct size. Maybe in gnrc_pktbuf_static.c:449?
|
Btw I don't know how to handle traffic from unknown interfaces. |
IMO, if there is no interface, then just skip the statistics for those packets. |
On the other hand, you could also aggregate the statistics for |
|
If we had a separate dedicated descriptors for pseudo interfaces (like loopback), netstats could help to find bugs like #5257. |
| module = NETSTATS_IPV6; | ||
| } | ||
| else { | ||
| printf("Module %s doesn't exist or do not provide statistics.\n", argv[3]); |
|
I guess for the first iteration everything seems fine. ACK once you addressed my comments. Please squash. |
|
Sorry for the delay. Addressed remaining issues and rebased to master. |
|
Can you squash then, please? |
OlegHahm
added
CI: ready for build
|
Squashed into a single commit. |
|
There is an error on some boards in the |
|
The last commit should fix it. |
|
okay to squash? |
|
Yes. ACK, as soon as Murdock is happy. |
3 participants
Based on #4801.