Re: reduce usage of mbuf cluster
On Wed, Jun 13, 2018 at 01:20:29PM +0900, YASUOKA Masahiko wrote:
> > The MCLGET() cannot be called anymore. Can we remove it and add a
> > compile time assert instead?
> >
> > CTASSERT(sizeof(struct ip) + sizeof(struct tcphdr) <= MHLEN);
> > CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct tcphdr) <= MHLEN);
>
> Yes, it's possible and it seems better.
>
> ok?
OK bluhm@
> Index: sys/netinet/tcp_subr.c
> ===
> RCS file: /cvs/src/sys/netinet/tcp_subr.c,v
> retrieving revision 1.171
> diff -u -p -r1.171 tcp_subr.c
> --- sys/netinet/tcp_subr.c8 May 2018 15:10:33 - 1.171
> +++ sys/netinet/tcp_subr.c13 Jun 2018 04:18:25 -
> @@ -191,6 +191,9 @@ tcp_template(struct tcpcb *tp)
> struct mbuf *m;
> struct tcphdr *th;
>
> + CTASSERT(sizeof(struct ip) + sizeof(struct tcphdr) <= MHLEN);
> + CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct tcphdr) <= MHLEN);
> +
> if ((m = tp->t_template) == 0) {
> m = m_get(M_DONTWAIT, MT_HEADER);
> if (m == NULL)
> @@ -208,19 +211,6 @@ tcp_template(struct tcpcb *tp)
> #endif /* INET6 */
> }
> m->m_len += sizeof (struct tcphdr);
> -
> - /*
> - * The link header, network header, TCP header, and TCP options
> - * all must fit in this mbuf. For now, assume the worst case of
> - * TCP options size. Eventually, compute this from tp flags.
> - */
> - if (m->m_len + MAX_TCPOPTLEN + max_linkhdr >= MHLEN) {
> - MCLGET(m, M_DONTWAIT);
> - if ((m->m_flags & M_EXT) == 0) {
> - m_free(m);
> - return (0);
> - }
> - }
> }
>
> switch(tp->pf) {
Re: reduce usage of mbuf cluster
On Wed, 13 Jun 2018 00:34:28 +0200
Alexander Bluhm wrote:
> On Tue, Jun 12, 2018 at 01:46:48PM +0900, YASUOKA Masahiko wrote:
>> > I found this because I wonder why my company, IIJ's HTTP proxy servers
>> > started using a lot of mbuf clusters after a certain version.
>
> Are you using IPv4 or IPv6?
IPv4 is used for that servers.
>> > + if (m->m_len + sizeof(struct tcphdr) >= MHLEN) {
>> is tweaked to
>> if (m->m_len + sizeof(struct tcphdr) > MHLEN) {
>
> Using > instead of >= is correct.
The condition was for MCLGET() not for keeping non-cluster.
> I don't understand why MAX_TCPOPTLEN and max_linkhdr was added to
> this calculation. The mbuf is just a chunk of memory that has to
> contain the data of the ip and tcp header. So removing all this
> seems reasonable.
I also can't find the reason of previous calculation.
> The MCLGET() cannot be called anymore. Can we remove it and add a
> compile time assert instead?
>
> CTASSERT(sizeof(struct ip) + sizeof(struct tcphdr) <= MHLEN);
> CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct tcphdr) <= MHLEN);
Yes, it's possible and it seems better.
ok?
Index: sys/netinet/tcp_subr.c
===
RCS file: /cvs/src/sys/netinet/tcp_subr.c,v
retrieving revision 1.171
diff -u -p -r1.171 tcp_subr.c
--- sys/netinet/tcp_subr.c 8 May 2018 15:10:33 - 1.171
+++ sys/netinet/tcp_subr.c 13 Jun 2018 04:18:25 -
@@ -191,6 +191,9 @@ tcp_template(struct tcpcb *tp)
struct mbuf *m;
struct tcphdr *th;
+ CTASSERT(sizeof(struct ip) + sizeof(struct tcphdr) <= MHLEN);
+ CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct tcphdr) <= MHLEN);
+
if ((m = tp->t_template) == 0) {
m = m_get(M_DONTWAIT, MT_HEADER);
if (m == NULL)
@@ -208,19 +211,6 @@ tcp_template(struct tcpcb *tp)
#endif /* INET6 */
}
m->m_len += sizeof (struct tcphdr);
-
- /*
-* The link header, network header, TCP header, and TCP options
-* all must fit in this mbuf. For now, assume the worst case of
-* TCP options size. Eventually, compute this from tp flags.
-*/
- if (m->m_len + MAX_TCPOPTLEN + max_linkhdr >= MHLEN) {
- MCLGET(m, M_DONTWAIT);
- if ((m->m_flags & M_EXT) == 0) {
- m_free(m);
- return (0);
- }
- }
}
switch(tp->pf) {
Re: reduce usage of mbuf cluster
On Wed, Jun 13, 2018 at 12:34:28AM +0200, Alexander Bluhm wrote:
> On Tue, Jun 12, 2018 at 01:46:48PM +0900, YASUOKA Masahiko wrote:
> > > I found this because I wonder why my company, IIJ's HTTP proxy servers
> > > started using a lot of mbuf clusters after a certain version.
>
> Are you using IPv4 or IPv6?
>
> > > + if (m->m_len + sizeof(struct tcphdr) >= MHLEN) {
> > is tweaked to
> > if (m->m_len + sizeof(struct tcphdr) > MHLEN) {
>
> Using > instead of >= is correct.
>
> I don't understand why MAX_TCPOPTLEN and max_linkhdr was added to
> this calculation. The mbuf is just a chunk of memory that has to
> contain the data of the ip and tcp header. So removing all this
> seems reasonable.
>
> The MCLGET() cannot be called anymore. Can we remove it and add a
> compile time assert instead?
>
> CTASSERT(sizeof(struct ip) + sizeof(struct tcphdr) <= MHLEN);
> CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct tcphdr) <= MHLEN);
>
I wonder if this TCP template is actually still an optimization or if
that was only true back in the 80is but now it no longer matters since
CPUs are much faster than memory.
Somebody should benchmark it :)
> > Index: sys/netinet/tcp_subr.c
> > ===
> > RCS file: /cvs/src/sys/netinet/tcp_subr.c,v
> > retrieving revision 1.171
> > diff -u -p -r1.171 tcp_subr.c
> > --- sys/netinet/tcp_subr.c 8 May 2018 15:10:33 - 1.171
> > +++ sys/netinet/tcp_subr.c 12 Jun 2018 04:43:18 -
> > @@ -207,20 +207,15 @@ tcp_template(struct tcpcb *tp)
> > break;
> > #endif /* INET6 */
> > }
> > - m->m_len += sizeof (struct tcphdr);
> >
> > - /*
> > -* The link header, network header, TCP header, and TCP options
> > -* all must fit in this mbuf. For now, assume the worst case of
> > -* TCP options size. Eventually, compute this from tp flags.
> > -*/
> > - if (m->m_len + MAX_TCPOPTLEN + max_linkhdr >= MHLEN) {
> > + if (m->m_len + sizeof(struct tcphdr) > MHLEN) {
> > MCLGET(m, M_DONTWAIT);
> > if ((m->m_flags & M_EXT) == 0) {
> > m_free(m);
> > return (0);
> > }
> > }
> > + m->m_len += sizeof(struct tcphdr);
> > }
> >
> > switch(tp->pf) {
>
--
:wq Claudio
Re: reduce usage of mbuf cluster
On Tue, Jun 12, 2018 at 01:46:48PM +0900, YASUOKA Masahiko wrote:
> > I found this because I wonder why my company, IIJ's HTTP proxy servers
> > started using a lot of mbuf clusters after a certain version.
Are you using IPv4 or IPv6?
> > + if (m->m_len + sizeof(struct tcphdr) >= MHLEN) {
> is tweaked to
> if (m->m_len + sizeof(struct tcphdr) > MHLEN) {
Using > instead of >= is correct.
I don't understand why MAX_TCPOPTLEN and max_linkhdr was added to
this calculation. The mbuf is just a chunk of memory that has to
contain the data of the ip and tcp header. So removing all this
seems reasonable.
The MCLGET() cannot be called anymore. Can we remove it and add a
compile time assert instead?
CTASSERT(sizeof(struct ip) + sizeof(struct tcphdr) <= MHLEN);
CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct tcphdr) <= MHLEN);
bluhm
> Index: sys/netinet/tcp_subr.c
> ===
> RCS file: /cvs/src/sys/netinet/tcp_subr.c,v
> retrieving revision 1.171
> diff -u -p -r1.171 tcp_subr.c
> --- sys/netinet/tcp_subr.c8 May 2018 15:10:33 - 1.171
> +++ sys/netinet/tcp_subr.c12 Jun 2018 04:43:18 -
> @@ -207,20 +207,15 @@ tcp_template(struct tcpcb *tp)
> break;
> #endif /* INET6 */
> }
> - m->m_len += sizeof (struct tcphdr);
>
> - /*
> - * The link header, network header, TCP header, and TCP options
> - * all must fit in this mbuf. For now, assume the worst case of
> - * TCP options size. Eventually, compute this from tp flags.
> - */
> - if (m->m_len + MAX_TCPOPTLEN + max_linkhdr >= MHLEN) {
> + if (m->m_len + sizeof(struct tcphdr) > MHLEN) {
> MCLGET(m, M_DONTWAIT);
> if ((m->m_flags & M_EXT) == 0) {
> m_free(m);
> return (0);
> }
> }
> + m->m_len += sizeof(struct tcphdr);
> }
>
> switch(tp->pf) {
Re: reduce usage of mbuf cluster
Let me update the diff
On Tue, 12 Jun 2018 13:39:51 +0900 (JST)
YASUOKA Masahiko wrote:
> Currently t_template of tcpcb is a mbuf cluster. This happened when
> max_linkhdr is increased from 16 to 64 (1.44 of
> sys/kern/uipc_domain.c).
>
> I found this because I wonder why my company, IIJ's HTTP proxy servers
> started using a lot of mbuf clusters after a certain version.
>
>
> ok?
>
> It doesn't seem to be necessary to have any extra space for the
> t_template of tcpcb.
> + if (m->m_len + sizeof(struct tcphdr) >= MHLEN) {
is tweaked to
if (m->m_len + sizeof(struct tcphdr) > MHLEN) {
ok?
Index: sys/netinet/tcp_subr.c
===
RCS file: /cvs/src/sys/netinet/tcp_subr.c,v
retrieving revision 1.171
diff -u -p -r1.171 tcp_subr.c
--- sys/netinet/tcp_subr.c 8 May 2018 15:10:33 - 1.171
+++ sys/netinet/tcp_subr.c 12 Jun 2018 04:43:18 -
@@ -207,20 +207,15 @@ tcp_template(struct tcpcb *tp)
break;
#endif /* INET6 */
}
- m->m_len += sizeof (struct tcphdr);
- /*
-* The link header, network header, TCP header, and TCP options
-* all must fit in this mbuf. For now, assume the worst case of
-* TCP options size. Eventually, compute this from tp flags.
-*/
- if (m->m_len + MAX_TCPOPTLEN + max_linkhdr >= MHLEN) {
+ if (m->m_len + sizeof(struct tcphdr) > MHLEN) {
MCLGET(m, M_DONTWAIT);
if ((m->m_flags & M_EXT) == 0) {
m_free(m);
return (0);
}
}
+ m->m_len += sizeof(struct tcphdr);
}
switch(tp->pf) {
reduce usage of mbuf cluster
Currently t_template of tcpcb is a mbuf cluster. This happened when
max_linkhdr is increased from 16 to 64 (1.44 of
sys/kern/uipc_domain.c).
I found this because I wonder why my company, IIJ's HTTP proxy servers
started using a lot of mbuf clusters after a certain version.
ok?
It doesn't seem to be necessary to have any extra space for the
t_template of tcpcb.
Index: sys/netinet/tcp_subr.c
===
RCS file: /cvs/src/sys/netinet/tcp_subr.c,v
retrieving revision 1.171
diff -u -p -r1.171 tcp_subr.c
--- sys/netinet/tcp_subr.c 8 May 2018 15:10:33 - 1.171
+++ sys/netinet/tcp_subr.c 12 Jun 2018 04:29:20 -
@@ -207,20 +207,15 @@ tcp_template(struct tcpcb *tp)
break;
#endif /* INET6 */
}
- m->m_len += sizeof (struct tcphdr);
- /*
-* The link header, network header, TCP header, and TCP options
-* all must fit in this mbuf. For now, assume the worst case of
-* TCP options size. Eventually, compute this from tp flags.
-*/
- if (m->m_len + MAX_TCPOPTLEN + max_linkhdr >= MHLEN) {
+ if (m->m_len + sizeof(struct tcphdr) >= MHLEN) {
MCLGET(m, M_DONTWAIT);
if ((m->m_flags & M_EXT) == 0) {
m_free(m);
return (0);
}
}
+ m->m_len += sizeof(struct tcphdr);
}
switch(tp->pf) {
