Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 14 Oct 2015 14:38:59 +0300
From:      Gleb Smirnoff <glebius@FreeBSD.org>
To:        Hiren Panchasara <hiren@FreeBSD.org>, Jonathan Looney <jlooney@juniper.net>
Cc:        svn-src-head@freebsd.org
Subject:   Re: svn commit: r289276 - in head/sys: conf kern netinet sys
Message-ID:  <20151014113859.GS1023@FreeBSD.org>
In-Reply-To: <201510140035.t9E0ZbXS030094@repo.freebsd.org>
References:  <201510140035.t9E0ZbXS030094@repo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
  Hi,

On Wed, Oct 14, 2015 at 12:35:37AM +0000, Hiren Panchasara wrote:
H> Author: hiren
H> Date: Wed Oct 14 00:35:37 2015
H> New Revision: 289276
H> URL: https://svnweb.freebsd.org/changeset/base/289276
H> 
H> Log:
H>   There are times when it would be really nice to have a record of the last few
H>   packets and/or state transitions from each TCP socket. That would help with
H>   narrowing down certain problems we see in the field that are hard to reproduce
H>   without understanding the history of how we got into a certain state. This
H>   change provides just that.

Thanks, Hiren for handling that, and thanks Jonathan for the patch.

I have a tiny stylistic suggestion:

H> Modified: head/sys/netinet/tcp_var.h
H> ==============================================================================
H> --- head/sys/netinet/tcp_var.h	Wed Oct 14 00:23:31 2015	(r289275)
H> +++ head/sys/netinet/tcp_var.h	Wed Oct 14 00:35:37 2015	(r289276)
H> @@ -37,6 +37,7 @@
H>  
H>  #ifdef _KERNEL
H>  #include <net/vnet.h>
H> +#include <sys/mbuf.h>
H>  
H>  /*
H>   * Kernel variables for tcp.
H> @@ -204,7 +205,17 @@ struct tcpcb {
H>  
H>  	uint32_t t_ispare[8];		/* 5 UTO, 3 TBD */
H>  	void	*t_pspare2[4];		/* 1 TCP_SIGNATURE, 3 TBD */
H> -	uint64_t _pad[6];		/* 6 TBD (1-2 CC/RTT?) */
H> +#if defined(_KERNEL) && defined(TCPPCAP)
H> +	struct mbufq t_inpkts;		/* List of saved input packets. */
H> +	struct mbufq t_outpkts;		/* List of saved output packets. */
H> +#ifdef _LP64
H> +	uint64_t _pad[0];		/* all used! */
H> +#else
H> +	uint64_t _pad[2];		/* 2 are available */
H> +#endif /* _LP64 */
H> +#else
H> +	uint64_t _pad[6];
H> +#endif /* defined(_KERNEL) && defined(TCPPCAP) */
H>  };

What if we write it down this way (thanks C11):

struct tcpcb {
	...
	union {
#ifdef TCPPCAP
		struct {
			struct mbufq t_inpkts;
			struct mbufq t_outpkts;
		};
#endif
		uint64_t _pad[6];
	}
};

So, compiler cares about pointer size, not us. And more readable, IMHO.

-- 
Totus tuus, Glebius.



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20151014113859.GS1023>