Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: l2: ethernet: arp: improve debug output #73115

Merged
merged 1 commit into from
May 27, 2024

Conversation

JordanYates
Copy link
Collaborator

Improve the ARP debug output by printing:

  • Sending ARP query
  • Queuing packets due to pending ARP query
  • Receiving ARP query response

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections to adding more prints, but imho these are more like debug messages instead of infos. If all is well and one is not debugging things, there should be no need to print anything. And with debugging one needs to usually enable the debug level which then prints these.

@JordanYates
Copy link
Collaborator Author

No objections to adding more prints, but imho these are more like debug messages instead of infos. If all is well and one is not debugging things, there should be no need to print anything. And with debugging one needs to usually enable the debug level which then prints these.

Is DBG the only valid log level in the networking stack? It seems I can turn on CONFIG_NET_LOG when I am having a problem and get 0% more idea of where my problem is until I turn on _LOG_LEVEL_DBG in the module that I don't yet know contains the problem.

If I change it to debug, there is no differentiation between "Here is the ARP request and response" which is 2 prints per IP and "Here is ARP translation for every single packet I ever send", which can immediately consume the entire log buffer.

If INFO is too verbose, my opinion is that either you should be turning off NET_LOG or the default level should be changed to WRN in Kconfig.

I won't die on this hill though :)

@jukkar
Copy link
Member

jukkar commented May 22, 2024

Yeah, you can turn off all network logging with CONFIG_NET_LOG and your workflow (setting CONFIG_NET_LOG=y) is one possibility to see output. Perhaps I am too close to this and usually never use NET_INFO as it has not been very useful for me because it can lead excessive printing. I usually always have set CONFIG_NET_LOG=y in my development and then set the log level to DBG to see in detail what is going on in the system.
Anyway, not sure what other people think about this, @rlubos WDYT?

@JordanYates
Copy link
Collaborator Author

Yeah, you can turn off all network logging with CONFIG_NET_LOG and your workflow (setting CONFIG_NET_LOG=y) is one possibility to see output. Perhaps I am too close to this and usually never use NET_INFO as it has not been very useful for me because it can lead excessive printing. I usually always have set CONFIG_NET_LOG=y in my development and then set the log level to DBG to see in detail what is going on in the system. Anyway, not sure what other people think about this, @rlubos WDYT?

Ignoring the networking part for now, my opinion is that if a piece of code is doing real work then there should be some level of logging at INFO. INFO should be for high level actions that let you know the thing is working normally, DBG is for the details that would otherwise spam the console. My 2c.

@jukkar
Copy link
Member

jukkar commented May 22, 2024

It is interesting to see other people workflow, the way you are using CONFIG_NET_LOG is kind of handy to see information about the normal state of the system. For me it probably means excessive logging but I can certainly cope this if we want to go to this direction. I checked subsys/net and there are quite many LOG_INF used, those should be converted to NET_INFO so that the CONFIG_NET_LOG would work as expected, I will send a PR for this at some point.

@rlubos
Copy link
Contributor

rlubos commented May 22, 2024

I kind of like the idea of introducing more granularity to the networking logs, currently in most cases it just DBG or ERR. And debug logs can really get bloated in some cases.
But IMO we should indeed consider changing the default log level in the networking subsystem to WARN, as suggested. If more components follow this pattern, the amount of logs enabled by default with INFO level could be too much.

Improve the ARP debug output by printing:
 * Sending ARP query
 * Queuing packets due to pending ARP query
 * Receiving ARP query response

Signed-off-by: Jordan Yates <jordan@embeint.com>
@JordanYates
Copy link
Collaborator Author

Changed NET_INFO to NET_DBG, changes to default log levels are outside the scope of the PR

@carlescufi carlescufi merged commit 251ddf0 into zephyrproject-rtos:main May 27, 2024
23 checks passed
@JordanYates JordanYates deleted the 240522_arp_debug branch May 27, 2024 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants