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

candump: epoll_wait() instead of select() runs faster and better keeps packet order #264

Merged
merged 1 commit into from
Dec 6, 2020

Conversation

IDC-Dragon
Copy link
Contributor

Using epoll_wait() instead of select() gives higher performance for listening on multiple interfaces. Additionally, the read order has a higher chance to resemble the true temporal order. select() gives implicit priority to the lower index socket.

@IDC-Dragon
Copy link
Contributor Author

select() is a horrible beast of the past. We should use epoll today, it is in the kernel longer than CAN support.
If you like the patch, I may change the other tools, too.

@hartkopp
Copy link
Member

hartkopp commented Dec 3, 2020

Hi Jörg,
many thanks for your effort!
I'll take a look at it and do some testing (which I assume you already did).
Btw. the commit message is completely broken. There is no Signed-off-by: tag and the commit message has a separated subject line and a desrciption with a line break at ~70 chars.
I applied your patch to my repo and fixed the commit message there:
hartkopp@4ca36df

Copy link
Member

@marckleinebudde marckleinebudde left a comment

Choose a reason for hiding this comment

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

Thanks for the patch, some nitpicks inline.

candump.c Outdated Show resolved Hide resolved
candump.c Outdated Show resolved Hide resolved
candump.c Outdated Show resolved Hide resolved
candump.c Outdated Show resolved Hide resolved
candump.c Outdated Show resolved Hide resolved
candump.c Outdated Show resolved Hide resolved
candump.c Outdated Show resolved Hide resolved
@marckleinebudde
Copy link
Member

marckleinebudde commented Dec 4, 2020

@IDC-Dragon, can you squash and force push the change, so that there is only one commit

@IDC-Dragon
Copy link
Contributor Author

Not sure if I did squash+push correct, don't see an effect on the pull request. I did:
git rebase -i origin/master~2 master
git push origin +master

@marckleinebudde
Copy link
Member

squahing means, make one commit from multiple ones. Your master branch still contains two commits.

@IDC-Dragon
Copy link
Contributor Author

I missed the squash in the rebase. Now it is one commit.

@ehartko
Copy link

ehartko commented Dec 4, 2020

Can you please change the commit message as suggested here:
#264 (comment)
git --amend is your friend ;-)

Copy link
Member

@marckleinebudde marckleinebudde left a comment

Choose a reason for hiding this comment

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

some nitpicks left

candump.c Outdated Show resolved Hide resolved
candump.c Outdated Show resolved Hide resolved
candump.c Outdated Show resolved Hide resolved
candump.c Outdated Show resolved Hide resolved
@IDC-Dragon
Copy link
Contributor Author

force-pushed again, it seems my editor messed the indentation in one place

@IDC-Dragon IDC-Dragon changed the title epoll_wait() instead of select() runs faster and keeps packet order better candump: epoll_wait() instead of select() runs faster and keeps packet order better Dec 4, 2020
@IDC-Dragon IDC-Dragon changed the title candump: epoll_wait() instead of select() runs faster and keeps packet order better candump: epoll_wait() instead of select() runs faster and better keeps packet order Dec 4, 2020
Copy link
Member

@marckleinebudde marckleinebudde left a comment

Choose a reason for hiding this comment

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

You have some idention problem still: feel free to squash my commit:

https://github.com/marckleinebudde/can-utils/commits/for-IDC-Dragon

@marckleinebudde
Copy link
Member

Now please rewrap you commit message to < 80 chars per line. Then we're good to go.

@IDC-Dragon
Copy link
Contributor Author

IDC-Dragon commented Dec 4, 2020

Now please rewrap you commit message to < 80 chars per line. Then we're good to go.

Done, I may shall say.

@marckleinebudde
Copy link
Member

It fails on android...

/home/travis/build/marckleinebudde/can-utils/candump.c:392:13: warning: implicit declaration of function 'epoll_create1' is invalid in C99 [-Wimplicit-function-declaration]
1086        fd_epoll = epoll_create1(0);

@marckleinebudde
Copy link
Member

marckleinebudde commented Dec 5, 2020

@marckleinebudde
Copy link
Member

epoll_create() works

@IDC-Dragon can you update the patch, then I'll merge it.

Using epoll_wait() instead of select() gives higher
performance for listening on multiple interfaces.
Additionally, the read order has a higher chance
to resemble the true temporal order.
select() gives implicit priority to the lower index socket.

Signed-off-by: Jörg Hohensohn <joerg.hohensohn@gmx.de>
@marckleinebudde marckleinebudde merged commit aa64110 into linux-can:master Dec 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants