Add sysctl to disable the TCP hostcache
ClosedPublic
Actions

Authored by j-nitrology.com on May 4 2016, 4:19 AM.

Details

Reviewers

hiren
rrs
sbruno
bz

Group Reviewers

transport

Commits

rS306464: This adds a sysctl which allows you to disable the TCP hostcache. This is handy

Summary

This adds a sysctl which allows you to disable the TCP hostcache. This is handy during testing of network related changes where cached entries may pollute your results, or during known congestion events where you don't want to unfairly penalize hosts.

Prior to r232346 this would have meant you would break any connection with a sub 1500 MTU, as the hostcache was authoritative. All entries as they stand today should simply be used to pre populate values for efficiency.

Test Plan

This has been running in production at Limelight

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 3575
Build 3615: arc lint + arc unit

Event Timeline

j-nitrology.com updated this revision to Diff 15882.May 4 2016, 4:19 AM

j-nitrology.com retitled this revision from to Add sysctl to disable the TCP hostcache.

j-nitrology.com updated this object.

j-nitrology.com edited the test plan for this revision. (Show Details)

j-nitrology.com added a reviewer: hiren.

Herald added a reviewer: transport. · View Herald TranscriptMay 4 2016, 4:19 AM

Herald added a subscriber: imp. · View Herald Transcript

j-nitrology.com added a comment.May 4 2016, 4:23 AM

This comment was removed by j-nitrology.com.

The default needs to maintain the current behavior

Herald edited edge metadata. · View Herald TranscriptMay 4 2016, 4:29 AM

bz requested changes to this revision.May 4 2016, 10:01 AM

bz added a reviewer: bz.

bz added a subscriber: bz.

bz added inline comments.

sys/netinet/tcp_hostcache.c
283	This needs to go after variable declarations
341	This needs to go after the variable declarations; same applies for all further down; I'll stop repeating myself ;)
470	Maybe we should also fix the comment to say zero instead of NULL

This revision now requires changes to proceed.May 4 2016, 10:01 AM

This seems like a good idea, but fixing the style bugs (e.g., local variable definitions should be at the start of the function before any executable code) is necessary.

Making it per-VNET caused me slight pondering, but seems like the right overall choice.

I'm glad that the description of the patch indicates that code calling into the cache has been inspected for poor interactions with the cache being disabled. I assume experimentation has taken place on both IPv4 and IPv6?

I've moved all functionality to post declaration and updated the comment about the return on tcp_hc_getmtu().
@rwatson yes I just figure it's a near free feature should you be using jails to do various testing. It has been tested with both v4 and v6 traffic.

j-nitrology.com updated this object.May 4 2016, 6:23 PM

j-nitrology.com edited edge metadata.

When someone has a moment, might this look good after the updates?

Seems like this is good to go in. @bz @rwatson ?

I'll go ahead with a commit in a couple days if I don't hear anything back. Thank you for looking into it.

All review comments have been addressed by submitter.

rrs accepted this revision.Jul 5 2016, 1:33 PM

rrs added a reviewer: rrs.

Closed by commit rS306464: This adds a sysctl which allows you to disable the TCP hostcache. This is handy (authored by hiren). · Explain WhySep 30 2016, 12:11 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

sys/

netinet/

tcp_hostcache.c

26 lines

Diff 15898

View Options

sys/netinet/tcp_hostcache.c

	Show First 20 Lines • Show All 2,966 Lines • ▼ Show 20 Lines
	*/			*/
	bool			bool
	vm_page_reclaim_contig_domain(int domain, int req, u_long npages,			vm_page_reclaim_contig_domain(int domain, int req, u_long npages,
	vm_paddr_t low, vm_paddr_t high, u_long alignment, vm_paddr_t boundary)			vm_paddr_t low, vm_paddr_t high, u_long alignment, vm_paddr_t boundary)
	{			{
	struct vm_domain *vmd;			struct vm_domain *vmd;
	vm_paddr_t curr_low;			vm_paddr_t curr_low;
	vm_page_t m_run, m_runs[NRUNS];			vm_page_t m_run, m_runs[NRUNS];
	u_long count, reclaimed;			u_long count, minalign, reclaimed;
	int error, i, options, req_class;			int error, i, options, req_class;

	KASSERT(npages > 0, ("npages is 0"));			KASSERT(npages > 0, ("npages is 0"));
	KASSERT(powerof2(alignment), ("alignment is not a power of 2"));			KASSERT(powerof2(alignment), ("alignment is not a power of 2"));
	KASSERT(powerof2(boundary), ("boundary is not a power of 2"));			KASSERT(powerof2(boundary), ("boundary is not a power of 2"));
				alcUnsubmitted Done Inline Actions I think that the rounding should be performed here, right after the KASSERT()s and before the sanity check on the number of free pages. The reason being that in the worst case vm_page_reclaim_run() will perform the rounded up number of page allocations before it frees any of the pages from the contiguous run. alc: I think that the rounding should be performed here, right after the KASSERT()s and before the…
	req_class = req & VM_ALLOC_CLASS_MASK;			req_class = req & VM_ALLOC_CLASS_MASK;

	/*			/*
	* The page daemon is allowed to dig deeper into the free page list.			* The page daemon is allowed to dig deeper into the free page list.
	*/			*/
	if (curproc == pageproc && req_class != VM_ALLOC_INTERRUPT)			if (curproc == pageproc && req_class != VM_ALLOC_INTERRUPT)
	req_class = VM_ALLOC_SYSTEM;			req_class = VM_ALLOC_SYSTEM;

	/*			/*
	* Return if the number of free pages cannot satisfy the requested			* Return if the number of free pages cannot satisfy the requested
	* allocation.			* allocation.
	*/			*/
	vmd = VM_DOMAIN(domain);			vmd = VM_DOMAIN(domain);
	count = vmd->vmd_free_count;			count = vmd->vmd_free_count;
	if (count < npages + vmd->vmd_free_reserved \|\| (count < npages +			if (count < npages + vmd->vmd_free_reserved \|\| (count < npages +
	vmd->vmd_interrupt_free_min && req_class == VM_ALLOC_SYSTEM) \|\|			vmd->vmd_interrupt_free_min && req_class == VM_ALLOC_SYSTEM) \|\|
	(count < npages && req_class == VM_ALLOC_INTERRUPT))			(count < npages && req_class == VM_ALLOC_INTERRUPT))
	return (false);			return (false);

				/*
				* The caller will attempt an allocation after some runs have been
				* reclaimed and added to the vm_phys buddy lists. To ensure that runs
				* are not fragmented, round up the requested length to the next power
				alcUnsubmitted Done Inline Actions Rather than "To ensure that runs are not fragmented, ...", I would say, "Due to limitations of vm_phys_alloc_contig(), ..." alc: Rather than "To ensure that runs are not fragmented, ...", I would say, "Due to limitations of…
				* or two or maximum chunk size, and ensure that each run is suitably
				* aligned.
				*/
				minalign = 1ul << imin(flsl(npages - 1), VM_NFREEORDER - 1);
				npages = roundup2(npages, minalign);
				if (alignment < ptoa(minalign))
				alignment = ptoa(minalign);

	/*			/*
	* Scan up to three times, relaxing the restrictions ("options") on			* Scan up to three times, relaxing the restrictions ("options") on
	* the reclamation of reservations and superpages each time.			* the reclamation of reservations and superpages each time.
	*/			*/
	for (options = VPSC_NORESERV;;) {			for (options = VPSC_NORESERV;;) {
	/*			/*
	* Find the highest runs that satisfy the given constraints			* Find the highest runs that satisfy the given constraints
	▲ Show 20 Lines • Show All 2,522 Lines • Show Last 20 Lines

Add sysctl to disable the TCP hostcacheClosedPublicActions