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

Simplify libp2p ResourceMgr computed defaults (no System.StreamsInbound limits and allow more System.ConnsInbound per MB) #9587

Closed
wants to merge 6 commits into from

Conversation

BigLep
Copy link
Contributor

@BigLep BigLep commented Jan 25, 2023

This is in response to libp2p/go-libp2p#2010 for simplification I think we want to be able to do for Kubo.

I believe will allow the resource manager to get out of the way more but still protect against script-kiddies opening many connections/streams against a node.

In order for this work, established (non-transient) connections will need register memory usage with the resource manager/accountant, ideally across all transports and muxers. Per libp2p/go-libp2p#2010 (comment), there are some cases where an established libp2p connection doesn't increase memory accounted for by the resource manager/accountant. In the meantime we can set System.ConssInbound based on ResourceMgr.MaxMemory and then strip this out when libp2p/go-libp2p#2010 lands.

@BigLep BigLep marked this pull request as draft January 25, 2023 00:24
@BigLep
Copy link
Contributor Author

BigLep commented Jan 25, 2023

Reviewers @MarcoPolo and @ajnavarro : please look at all of createDefaultLimitConfig. I thought showing it with code was quicker than typing into libp2p/go-libp2p#2010.

This is not intended to be merged as is, but rather facilitate the discussion.

@BigLep BigLep changed the title [DO NOT MERGE] Demonstrating what I think we want for Kubo's simplified resource manager setup Simplify libp2p ResourceMgr computed defaults (no StreamInbound limits and allow more System.ConnsInbound per MB) Jan 26, 2023
@BigLep BigLep changed the title Simplify libp2p ResourceMgr computed defaults (no StreamInbound limits and allow more System.ConnsInbound per MB) Simplify libp2p ResourceMgr computed defaults (no System.StreamsInbound limits and allow more System.ConnsInbound per MB) Jan 26, 2023
@BigLep
Copy link
Contributor Author

BigLep commented Jan 26, 2023

@ajnavarro : I think this is closer to something we'd want to merge for 0.18.1. If you agree, feel free to take over and merge.

@BigLep BigLep mentioned this pull request Jan 26, 2023
@ajnavarro
Copy link
Member

I was thinking: 1Mb per connection isn't too much?

@MarcoPolo
Copy link
Contributor

I was thinking: 1Mb per connection isn't too much?

Depends on the latency. If you have a 80 Mbit/s connection and a latency of 100ms, then your buffer should be about 1MiB for optimal throughput. 1MiB is how much data is inflight before being acknowledged, it’s the bandwidth-delay product.

We should (and I think we already do in go-libp2p) reserve more memory as we need more buffer space.

@BigLep
Copy link
Contributor Author

BigLep commented Jan 26, 2023

@ajnavarro : I have incorporated the comments and I think gotten this into a better state. (My intent was to get this conceptually right. I'm sure I have broken Go code here.). Can you please take this over to get it across the line?

@BigLep BigLep marked this pull request as ready for review January 26, 2023 16:56
@@ -0,0 +1,36 @@
# Kubo changelog v0.18

## v0.18.1
Copy link
Member

@lidel lidel Jan 26, 2023

Choose a reason for hiding this comment

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

nit: we dont want a separate file, let's move this to docs/changelogs/v0.18, like we did with 0.13.1.

@ajnavarro I've already added my changes to 0.18.1 section in #9543 which is in master now, feel free to reuse

@ajnavarro
Copy link
Member

ajnavarro commented Jan 27, 2023

Superseded by #9593

@ajnavarro ajnavarro closed this Jan 27, 2023
@hacdias hacdias deleted the biglep/resource-manager-example-of-what-want branch May 9, 2023 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants