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

Update collectd and graphite UDP listeners with perf enhancements #4681

Merged
merged 3 commits into from
Nov 10, 2015

Conversation

sparrc
Copy link
Contributor

@sparrc sparrc commented Nov 5, 2015

closes #4678

related to #4676

cc @otoolep

@@ -53,7 +53,7 @@ type Service struct {
wg sync.WaitGroup
err chan error
stop chan struct{}
ln *net.UDPConn
conn *net.UDPConn
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed this just for consistency with graphite and udp services

@sparrc
Copy link
Contributor Author

sparrc commented Nov 5, 2015

@otoolep
Copy link
Contributor

otoolep commented Nov 5, 2015

We need the CHANGELOG updated, and https://github.com/influxdb/influxdb/blob/master/etc/config.sample.toml updated to include these new values.

@otoolep
Copy link
Contributor

otoolep commented Nov 5, 2015

CHANGELOG should be updated in the "Features" section -- allow read buffer size to be set.

DefaultBatchDuration = toml.Duration(10 * time.Second)

DefaultTypesDB = "/usr/share/collectd/types.db"

// DefaultUDPReadBuffer is the default UDP read buffer
// increasing this increases the number of UDP packets that can be handled.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little vague. Why not be precise and repeat the text in the Go docs?

"Sets the size of the operating system's receive buffer associated with the UDP traffic "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kk 👍

@otoolep
Copy link
Contributor

otoolep commented Nov 5, 2015

Does the buffer size apply to TCP traffic? If so, we should set it for Graphite TCP, right?

@sparrc
Copy link
Contributor Author

sparrc commented Nov 5, 2015

For graphite we can't change the tcp read buffer at the moment. We would have the change the net.Listen call to a net.DialTCP call to get that functionality.

@dswarbrick
Copy link

Somewhere deep down, SetReadBuffer() will be doing the equivalent of a setsockopt() with SO_RCVBUF option. Note that Linux (presumably other *nixes) has maximums that this can be set to. The following are from my workstation, running kernel 4.2:

net.core.rmem_default = 212992
net.core.rmem_max = 212992
net.ipv4.tcp_rmem = 4096    87380   6291456

These can of course be modified with sysctl -w ...

@nkatsaros
Copy link
Contributor

You can do tcpConn, ok := conn.(*net.TCPConn) and do whatever you want on the connection that s.ln.Accept() spits out. There's no need to stray from the generic Listen/Accept/Dial functions in the net package.

@sparrc
Copy link
Contributor Author

sparrc commented Nov 5, 2015

@dswarbrick That is also what I get:

$ sudo sysctl -a | grep net.core.rmem
net.core.rmem_default = 212992
net.core.rmem_max = 212992

maybe it can adjust the max? I'm not sure...

@sparrc
Copy link
Contributor Author

sparrc commented Nov 5, 2015

@nkatsaros, from what I understand, net.Listen("tcp"...) is a simplified alternative to running net.DialTCP(...). I was just saying that it would be easier to adjust this if we used DialTCP, I suppose we could typecast and adjust on every for-loop iteration but that seems messy.

@dswarbrick
Copy link

@sparrc Need to be root to modify the OS defaults / max. These should not be fiddled with directly by InfluxDB, as they will affect all future sockets, in all applications. It would be worth documenting how to change them, or linking to relevant documentation.

@sparrc
Copy link
Contributor Author

sparrc commented Nov 5, 2015

oh, looks like BSD already has the max set higher, that would explain why it worked

OSX 10.11 (8MB)

$ sysctl kern.ipc.maxsockbuf
kern.ipc.maxsockbuf: 8388608

FreeBSD 10.1 (2MB)

$ sysctl kern.ipc.maxsockbuf
kern.ipc.maxsockbuf: 2097152

Get it together Linux!!

I'll put a note in the documentation

@sparrc sparrc force-pushed the udp-read-buffer branch 2 times, most recently from f1eafb1 to 3696213 Compare November 5, 2015 22:50
@otoolep
Copy link
Contributor

otoolep commented Nov 6, 2015

Thanks for the heads-up @dswarbrick

@sparrc -- looks like we have documentation issue here.

@sparrc sparrc force-pushed the udp-read-buffer branch 3 times, most recently from 716768e to 23510bd Compare November 6, 2015 17:10
@sparrc sparrc force-pushed the udp-read-buffer branch 6 times, most recently from 0bff268 to 6b37d29 Compare November 7, 2015 00:15

### BSD/Darwin

On BSD/Darwin systems you need to add about a 15% padding to the kernel 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.

@otoolep finally tracked down the BSD/Darwin failures I was seeing related to this. This PR should be good to merge now.

@sparrc sparrc merged commit 9625953 into master Nov 10, 2015
@sparrc sparrc deleted the udp-read-buffer branch November 10, 2015 20:27
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.

Add SetReadBuffer() to collectd and Graphite
5 participants