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.

Sign up for GitHub

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

packet drop issue resolution #30

Closed
wants to merge 1 commit into from
Closed

packet drop issue resolution #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions core/modules/measure.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,6 @@ void Measure::ProcessBatch(Context *ctx, bess::PacketBatch *batch) {
uint64_t now_ns = tsc_to_ns(rdtsc());
size_t offset = offset_;

mcslock_node_t mynode;
mcs_lock(&lock_, &mynode);

pkt_cnt_ += batch->cnt();

int cnt = batch->cnt();
Expand All @@ -132,21 +129,20 @@ void Measure::ProcessBatch(Context *ctx, bess::PacketBatch *batch) {

bytes_cnt_ += batch->pkts()[i]->total_len();

rtt_hist_.Insert(diff);
rtt_hist_.AtomicInsert(diff);
if (rand_.GetRealNonzero() <= jitter_sample_prob_) {
if (unlikely(!last_rtt_ns_)) {
last_rtt_ns_ = diff;
continue;
}
uint64_t jitter = absdiff(diff, last_rtt_ns_);
jitter_hist_.Insert(jitter);
int64_t jitter = diff - last_rtt_ns_;
jitter = jitter * ((jitter>0) - (jitter<0));
Comment on lines +138 to +139
Copy link
Contributor

@gab-arrobo gab-arrobo Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why not just use the abs?

Suggested change
int64_t jitter = diff - last_rtt_ns_;
jitter = jitter * ((jitter>0) - (jitter<0));
int64_t jitter = abs(diff - last_rtt_ns_);

Of course, the above would need to include <cstdlib> if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

suggested change in line 138 wont work/compile as abs dnt work on atomics. latest patch has that change.

Comment on lines +138 to +139
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to apply clang-format to address the GHA issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

covered in latest pr

Comment on lines +138 to +139
Copy link
Contributor

@gab-arrobo gab-arrobo Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Moreover, why do you have to change jitter to int64_t given that the argument for jitter_hist_.AtomicInsert() is uint64_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this context, suggested change wont work as uint64_t wont be able to save negative values .

jitter_hist_.AtomicInsert(jitter);
last_rtt_ns_ = diff;
}
}
}

mcs_unlock(&lock_, &mynode);

RunNextModule(ctx, batch);
}

Expand Down
12 changes: 4 additions & 8 deletions core/modules/measure.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
#include "../utils/histogram.h"
#include "../utils/mcslock.h"
#include "../utils/random.h"

#include <atomic>
class Measure final : public Module {
public:
Measure(uint64_t ns_per_bucket = 1, uint64_t max_ns = 0)
Expand Down Expand Up @@ -69,22 +69,18 @@ class Measure final : public Module {
static const uint64_t kDefaultNsPerBucket = 100;
static const uint64_t kDefaultMaxNs = 100'000'000; // 100 ms
static constexpr double kDefaultIpDvSampleProb = 0.05;

void Clear();

Histogram<uint64_t> rtt_hist_;
Histogram<uint64_t> jitter_hist_;

Random rand_;
double jitter_sample_prob_;
uint64_t last_rtt_ns_;

std::atomic<std::uint64_t> last_rtt_ns_;
size_t offset_; // in bytes
int attr_id_;

uint64_t pkt_cnt_;
uint64_t bytes_cnt_;

std::atomic<std::uint64_t> pkt_cnt_;
std::atomic<std::uint64_t> bytes_cnt_;
mcslock lock_;
};

Expand Down
Loading