Skip to content

Conversation

@alfreb
Copy link
Contributor

@alfreb alfreb commented Dec 3, 2018

WIP: Adjusting TCP buffer usage and receive window size

@jenkins-includeos
Copy link

Can one of the admins verify this patch with one of the following commands:

  • "jenkins test please" for a one time test run
  • "ok to test" to accept latest and future commits on this pull request for testing
  • "add to whitelist" to add the author to the whitelist

Copy link
Contributor

@KristianJerpetjon KristianJerpetjon left a comment

Choose a reason for hiding this comment

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

mostly trivial comments feel free to improve them if you want to

inline bool empty();

/** Fires when the resource has been full and is not full anymore **/
void on_non_full(Event e){ non_full = e; }
Copy link
Contributor

Choose a reason for hiding this comment

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

non or not ?
Do we have a naming convention ?

Copy link
Member

Choose a reason for hiding this comment

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

on_full_changed maybe?


Packet_view_ptr Connection::create_outgoing_packet()
{
update_rcv_wnd();
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we know what interface you are updating the rcw wnd on ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's an update on the receive window value for that Connection.

// x-rtx_q.size(), rtx_q.size());
}

uint32_t Connection::calculate_rcv_wnd() const
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be inlined ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Inlining it means rebuilding half the OS when tampering with it. I wouldn't say the implementation is 100% yet.
Also, won't the compiler inline it for us inside?

// I also think we shouldn't reach this point due to State::check_seq checking
// if we're inside the window. if packet is out of order tho we can change the RCV wnd (i think).
if(UNLIKELY(bufalloc->allocatable() < host_.max_bufsize())) {
/*if(UNLIKELY(bufalloc->allocatable() < host_.max_bufsize())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I 👀 dead code

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree - we can probably throw away this part now.


debug2("<Connection::retransmit> With data (wq.sz=%u) buf.unacked=%u\n",
writeq.size(), buf.length() - buf.acknowledged);
writeq.size(), buf->size(), buf->size() - writeq.acked());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a 🐦 ? is it a ✈️ ?

no its a 🌮 😄

#include <common.cxx>
#include <util/alloc_pmr.hpp>
#include <util/units.hpp>

Copy link
Contributor

Choose a reason for hiding this comment

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

Have we tried this towards libcxx >= 7.0 ?

KristianJerpetjon and others added 28 commits January 15, 2019 14:01
Test: Set a timeout for microlb get connection
…chother resulting in os block not getting time

Co-authored-by: Martin Nordsletten <mnordsletten@gmail.com>
@KristianJerpetjon KristianJerpetjon merged commit cc87322 into includeos:v0.13.x Jan 22, 2019
@KristianJerpetjon
Copy link
Contributor

Since this PR now successfully builds on vaskemaskin it gets merged

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.

6 participants