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

Fixing issue #215 #449

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

LocutusOfBorg
Copy link
Contributor

I don't know, does this code makes sense?

I would like to test it, but I don't know how to do it properly

src/ec_decode.c Outdated
{
/* calculate the stats */
stats_half_end(&GBL_STATS->bh, pkthdr->caplen);
count = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this cause stats_half_start() and stats_half_end() to be called without processing any additional packets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, so how to solve this? Adding packets in a queue?

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about calling stats_start when count is 0 and end when it has reached the packet limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, I hope
but this solution needs indeed LOT of testing

@eaescob
Copy link
Contributor

eaescob commented Jan 8, 2014

I don't think travis actually failed, a build stalled.

@LocutusOfBorg
Copy link
Contributor Author

In the last few days travis has needed a lot of rebuilds, and is using two concurrent builds instead of 5... I think there is some overread in the system! Anyway, I restarted the build!

@koeppea
Copy link
Member

koeppea commented Mar 6, 2018

Rebased onto current master and renamed commit.
Need to test if it really has a positive effect when ettercap has to serve some bandwidth.

@koeppea
Copy link
Member

koeppea commented Apr 30, 2018

Rebased over latest master and resolved conflicts + two errors.
Testing now the impact on transfer performance...

@koeppea
Copy link
Member

koeppea commented May 19, 2018

Comparism test done:

1GByte via FTP from one VM(FreeBSD) to another VM(Debian 64-bit) on the same host
w/o Ettercap(IPv6):         duration: 00:50 (19.68 MiB/s)
w/o Ettercap(IPv4):         duration: 00:46 (21.63 MiB/s)
w/  Ettercap(master/IPv6):  duration: 04:45 ( 3.50 MiB/s)
w/  Ettercap(master/IPv4):  duration: 04:25 ( 3.76 MiB/s)
w/  Ettercap(fix-215/IPv6): duration: 04:31 ( 3.68 MiB/s)
w/  Ettercap(fix-215/IPv4): duration: 04:44 ( 3.52 MiB/s)

I also promised myself more out of it. However, since it's improving in both cases (IPv4 & IPv6) against master, I vote for merge in any case.

@koeppea
Copy link
Member

koeppea commented May 19, 2018

Sorry I'fuzzed up.
IPv4 even worse. Maybe random. I think the pulll request doesn't improve the performance of Ettercap, unfortunately.

@sgeto
Copy link
Contributor

sgeto commented May 21, 2018

IPv4 even worse.

Worse than before applying this PR?

@koeppea
Copy link
Member

koeppea commented May 25, 2018

Well I think this PR may have an effect on performance.
But at the moment the issue lies somewhere else. I'm afraid now it's time to profile Ettercap to find the real bottleneck. When this is found and potentially fixed, this PR could make a difference, then relativiately to the improved throughput.

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