Date: Tue, 15 Jan 2013 17:56:17 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Hans Petter Selasky <hselasky@FreeBSD.org> Subject: Re: svn commit: r245222 - head/sys/sys Message-ID: <20130115135617.GJ79056@FreeBSD.org> In-Reply-To: <20130115220100.G977@besplex.bde.org> References: <201301090909.r09999kV013794@svn.freebsd.org> <20130109091217.GL66284@FreeBSD.org> <20130110081336.L967@besplex.bde.org> <20130115104248.GI79056@FreeBSD.org> <20130115220100.G977@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--BwCQnh7xodEAoBMC
Content-Type: text/plain; charset=koi8-r
Content-Disposition: inline
Bruce,
On Tue, Jan 15, 2013 at 11:13:36PM +1100, Bruce Evans wrote:
B> > can you please look at attached patch, that fixes problems you
B> > describe? It
B> >
B> > - fixes mentioned style bugs in param.h
B> > - adds int casts to MHLEN and MLEN
B> > - removes extra casts from (void *) to (struct mbuf *)
B> > - removes extra declarations of inlined functions
B> > - uninlines and moves m_get2() and m_getjcl() to uipc_mbuf.c
B> > - size argument of m_get2() swicthed back to int
B>
B> Looks mostly good.
B>
B> % ...
B> % Index: sys/mbuf.h
B> % ===================================================================
B> % --- sys/mbuf.h (revision 245450)
B> % +++ sys/mbuf.h (working copy)
B> % @@ -52,11 +52,14 @@
B> % * stored. Additionally, it is possible to allocate a separate buffer
B> % * externally and attach it to the mbuf in a way similar to that of mbuf
B> % * clusters.
B> % + *
B> % + * MLEN is data length in a normal mbuf.
B> % + * MHLEN is data length in an mbuf with pktheader.
B> % + * MINCLSIZE is a smallest amount of data that should be put into cluster.
B> % */
B>
B> The comment needs indent protection if you want to preserve the line
B> structure of the new comments. I don't see any better way to format these
B> lines as bullet points. A separate paragraph for each would be too verbose.
B>
B> % -#define MLEN (MSIZE - sizeof(struct m_hdr)) /* normal data len */
B> % -#define MHLEN (MLEN - sizeof(struct pkthdr)) /* data len w/pkthdr */
B> % -#define MINCLSIZE (MHLEN + 1) /* smallest amount to put in cluster */
B> % -#define M_MAXCOMPRESS (MHLEN / 2) /* max amount to copy for compression */
B> % +#define MLEN ((int)(MSIZE - sizeof(struct m_hdr)))
B> % +#define MHLEN ((int)(MLEN - sizeof(struct pkthdr)))
B> % +#define MINCLSIZE (MHLEN + 1)
B>
B> These never needed to be sorted non-alphabetically.
Sorting alphabetically moves MLEN to the bottom, but the above macros
are defined via MLEN. IMO, it is more easy to read when we move from
most simple macro to ones that are derived from it, not in alphabetical
order.
B> I hope this doesn't cause any problems. MHLEN, etc., are used just a few
B> times outside of mbuf.h where we can't control this, mostly in specialized
B> code. One interesting use in non-specialized code is in tcp_output():
B>
B> if (len <= MHLEN - hdrlen - max_linkhdr) {
B>
B> Here len has type long, MHLEN has type size_t (before this change) and type
B> int (after this change), hdrlen has type unsigned (misspelled as that instead
B> of u_int), and max_linkhdr has type int. So the type combinations are
B> nonsense, and there is a good chance of getting different compiler warnings
B> about this before and after the change. Having just one unsigned type in
B> the mix tends to promote everything to unsigned, except on 64-bit systems
B> with one u_int but no size_t, the long has highest rank so everything
B> gets promoted to long.
I think here we can do:
- unsigned ipoptlen, optlen, hdrlen;
+ int ipoptlen, optlen, hdrlen;
These three take their values from:
- ip6_optlen()
- m_len (from mbuf header)
- tcp_addoptions()
... , which all are ints.
- sizeof(), which can be casted.
I left the ipsec_optlen unsigned. Its type originates from
ipsec_hdrsiz_internal(), which returns result of sizeof().
Patch attached.
This leads to (len <= MHLEN - hdrlen - max_linkhdr) comparing
long and (int - int - int). This also fixes many other places in
tcp_output, where values od ipoptlen, optlen, hdrlen are assigned
to int variables or supplied as int arguments.
B> % ...
B> % @@ -393,23 +396,10 @@
B> % extern uma_zone_t zone_jumbo16;
B> % extern uma_zone_t zone_ext_refcnt;
B> %
B> % -static __inline struct mbuf *m_getcl(int how, short type, int flags);
B> % ...
B> % -static __inline void *m_cljget(struct mbuf *m, int how, int size);
B> % -static __inline void m_chtype(struct mbuf *m, short new_type);
B> % -void mb_free_ext(struct mbuf *);
B> % -static __inline struct mbuf *m_last(struct mbuf *m);
B> % -int m_pkthdr_init(struct mbuf *m, int how);
B> % +struct mbuf *m_get2(int how, short type, int flags, int size);
B> % +struct mbuf *m_getjcl(int how, short type, int flags, int size);
B> % +void mb_free_ext(struct mbuf *);
B>
B> This one is still missing a parameter name, unlike all (?) the others.
B>
B> % +int m_pkthdr_init(struct mbuf *m, int how);
B>
B> The 2 new non-inline prototypes should be moved to the general section
B> for non-inline prototypes. The 2 old ones must remain early since
B> they are used in the inlines.
B>
B> After moving the 2 new ones, also remove their parameter names to match
B> the (worse) nearby style. This leaves only 1 nearby other for mb_free_ext()
B> to be mismatched with.
B>
B> The general prototype section has a fairly uniform style, with the only
B> obvious bugs being:
B> - m_copyup() has parameter names
B> - m_sanity() is misindented by 1 space
B> - m_unshare() has 1 parameter name but 2 parameters.
Applied, patch attached.
--
Totus tuus, Glebius.
--BwCQnh7xodEAoBMC
Content-Type: text/x-diff; charset=koi8-r
Content-Disposition: attachment; filename="tcp_output.diff"
Index: netinet/tcp_output.c
===================================================================
--- netinet/tcp_output.c (revision 245450)
+++ netinet/tcp_output.c (working copy)
@@ -173,7 +173,7 @@
struct ipovly *ipov = NULL;
struct tcphdr *th;
u_char opt[TCP_MAXOLEN];
- unsigned ipoptlen, optlen, hdrlen;
+ int ipoptlen, optlen, hdrlen;
#ifdef IPSEC
unsigned ipsec_optlen = 0;
#endif
@@ -684,10 +684,10 @@
optlen = 0;
#ifdef INET6
if (isipv6)
- hdrlen = sizeof (struct ip6_hdr) + sizeof (struct tcphdr);
+ hdrlen = (int)(sizeof(struct ip6_hdr) + sizeof(struct tcphdr));
else
#endif
- hdrlen = sizeof (struct tcpiphdr);
+ hdrlen = (int)sizeof(struct tcpiphdr);
/*
* Compute options for segment.
--BwCQnh7xodEAoBMC
Content-Type: text/x-diff; charset=koi8-r
Content-Disposition: attachment; filename="mbuf.diff"
Index: netinet/tcp_output.c
===================================================================
--- netinet/tcp_output.c (revision 245450)
+++ netinet/tcp_output.c (working copy)
@@ -173,7 +173,7 @@
struct ipovly *ipov = NULL;
struct tcphdr *th;
u_char opt[TCP_MAXOLEN];
- unsigned ipoptlen, optlen, hdrlen;
+ int ipoptlen, optlen, hdrlen;
#ifdef IPSEC
unsigned ipsec_optlen = 0;
#endif
@@ -684,10 +684,10 @@
optlen = 0;
#ifdef INET6
if (isipv6)
- hdrlen = sizeof (struct ip6_hdr) + sizeof (struct tcphdr);
+ hdrlen = (int)(sizeof(struct ip6_hdr) + sizeof(struct tcphdr));
else
#endif
- hdrlen = sizeof (struct tcpiphdr);
+ hdrlen = (int)sizeof(struct tcpiphdr);
/*
* Compute options for segment.
Index: sys/param.h
===================================================================
--- sys/param.h (revision 245450)
+++ sys/param.h (working copy)
@@ -156,8 +156,8 @@
* MCLBYTES must be no larger than PAGE_SIZE.
*/
#ifndef MSIZE
-#define MSIZE 256 /* size of an mbuf */
-#endif /* MSIZE */
+#define MSIZE 256 /* size of an mbuf */
+#endif
#ifndef MCLSHIFT
#define MCLSHIFT 11 /* convert bytes to mbuf clusters */
Index: sys/mbuf.h
===================================================================
--- sys/mbuf.h (revision 245450)
+++ sys/mbuf.h (working copy)
@@ -52,11 +52,14 @@
* stored. Additionally, it is possible to allocate a separate buffer
* externally and attach it to the mbuf in a way similar to that of mbuf
* clusters.
+ *
+ * MLEN is data length in a normal mbuf.
+ * MHLEN is data length in an mbuf with pktheader.
+ * MINCLSIZE is a smallest amount of data that should be put into cluster.
*/
-#define MLEN (MSIZE - sizeof(struct m_hdr)) /* normal data len */
-#define MHLEN (MLEN - sizeof(struct pkthdr)) /* data len w/pkthdr */
-#define MINCLSIZE (MHLEN + 1) /* smallest amount to put in cluster */
-#define M_MAXCOMPRESS (MHLEN / 2) /* max amount to copy for compression */
+#define MLEN ((int)(MSIZE - sizeof(struct m_hdr)))
+#define MHLEN ((int)(MLEN - sizeof(struct pkthdr)))
+#define MINCLSIZE (MHLEN + 1)
#ifdef _KERNEL
/*-
@@ -393,23 +396,8 @@
extern uma_zone_t zone_jumbo16;
extern uma_zone_t zone_ext_refcnt;
-static __inline struct mbuf *m_getcl(int how, short type, int flags);
-static __inline struct mbuf *m_get(int how, short type);
-static __inline struct mbuf *m_get2(int how, short type, int flags,
- u_int size);
-static __inline struct mbuf *m_gethdr(int how, short type);
-static __inline struct mbuf *m_getjcl(int how, short type, int flags,
- int size);
-static __inline struct mbuf *m_getclr(int how, short type); /* XXX */
-static __inline int m_init(struct mbuf *m, uma_zone_t zone,
- int size, int how, short type, int flags);
-static __inline struct mbuf *m_free(struct mbuf *m);
-static __inline void m_clget(struct mbuf *m, int how);
-static __inline void *m_cljget(struct mbuf *m, int how, int size);
-static __inline void m_chtype(struct mbuf *m, short new_type);
-void mb_free_ext(struct mbuf *);
-static __inline struct mbuf *m_last(struct mbuf *m);
-int m_pkthdr_init(struct mbuf *m, int how);
+void mb_free_ext(struct mbuf *);
+int m_pkthdr_init(struct mbuf *, int);
static __inline int
m_gettype(int size)
@@ -502,7 +490,7 @@
args.flags = 0;
args.type = type;
- return ((struct mbuf *)(uma_zalloc_arg(zone_mbuf, &args, how)));
+ return (uma_zalloc_arg(zone_mbuf, &args, how));
}
/*
@@ -529,7 +517,7 @@
args.flags = M_PKTHDR;
args.type = type;
- return ((struct mbuf *)(uma_zalloc_arg(zone_mbuf, &args, how)));
+ return (uma_zalloc_arg(zone_mbuf, &args, how));
}
static __inline struct mbuf *
@@ -539,87 +527,9 @@
args.flags = flags;
args.type = type;
- return ((struct mbuf *)(uma_zalloc_arg(zone_pack, &args, how)));
+ return (uma_zalloc_arg(zone_pack, &args, how));
}
-/*
- * m_get2() allocates minimum mbuf that would fit "size" argument.
- *
- * XXX: This is rather large, should be real function maybe.
- */
-static __inline struct mbuf *
-m_get2(int how, short type, int flags, u_int size)
-{
- struct mb_args args;
- struct mbuf *m, *n;
- uma_zone_t zone;
-
- args.flags = flags;
- args.type = type;
-
- if (size <= MHLEN || (size <= MLEN && (flags & M_PKTHDR) == 0))
- return ((struct mbuf *)(uma_zalloc_arg(zone_mbuf, &args, how)));
- if (size <= MCLBYTES)
- return ((struct mbuf *)(uma_zalloc_arg(zone_pack, &args, how)));
-
- if (size > MJUM16BYTES)
- return (NULL);
-
- m = uma_zalloc_arg(zone_mbuf, &args, how);
- if (m == NULL)
- return (NULL);
-
-#if MJUMPAGESIZE != MCLBYTES
- if (size <= MJUMPAGESIZE)
- zone = zone_jumbop;
- else
-#endif
- if (size <= MJUM9BYTES)
- zone = zone_jumbo9;
- else
- zone = zone_jumbo16;
-
- n = uma_zalloc_arg(zone, m, how);
- if (n == NULL) {
- uma_zfree(zone_mbuf, m);
- return (NULL);
- }
-
- return (m);
-}
-
-/*
- * m_getjcl() returns an mbuf with a cluster of the specified size attached.
- * For size it takes MCLBYTES, MJUMPAGESIZE, MJUM9BYTES, MJUM16BYTES.
- *
- * XXX: This is rather large, should be real function maybe.
- */
-static __inline struct mbuf *
-m_getjcl(int how, short type, int flags, int size)
-{
- struct mb_args args;
- struct mbuf *m, *n;
- uma_zone_t zone;
-
- if (size == MCLBYTES)
- return m_getcl(how, type, flags);
-
- args.flags = flags;
- args.type = type;
-
- m = uma_zalloc_arg(zone_mbuf, &args, how);
- if (m == NULL)
- return (NULL);
-
- zone = m_getzone(size);
- n = uma_zalloc_arg(zone, m, how);
- if (n == NULL) {
- uma_zfree(zone_mbuf, m);
- return (NULL);
- }
- return (m);
-}
-
static __inline void
m_free_fast(struct mbuf *m)
{
@@ -881,7 +791,7 @@
int, int, int, int);
struct mbuf *m_copypacket(struct mbuf *, int);
void m_copy_pkthdr(struct mbuf *, struct mbuf *);
-struct mbuf *m_copyup(struct mbuf *n, int len, int dstoff);
+struct mbuf *m_copyup(struct mbuf *, int, int);
struct mbuf *m_defrag(struct mbuf *, int);
void m_demote(struct mbuf *, int);
struct mbuf *m_devget(char *, int, int, struct ifnet *,
@@ -891,6 +801,8 @@
u_int m_fixhdr(struct mbuf *);
struct mbuf *m_fragment(struct mbuf *, int, int);
void m_freem(struct mbuf *);
+struct mbuf *m_get2(int, short, int, int);
+struct mbuf *m_getjcl(int, short, int, int);
struct mbuf *m_getm2(struct mbuf *, int, int, short, int);
struct mbuf *m_getptr(struct mbuf *, int, int *);
u_int m_length(struct mbuf *, struct mbuf **);
@@ -900,10 +812,10 @@
void m_print(const struct mbuf *, int);
struct mbuf *m_pulldown(struct mbuf *, int, int, int *);
struct mbuf *m_pullup(struct mbuf *, int);
-int m_sanity(struct mbuf *, int);
+int m_sanity(struct mbuf *, int);
struct mbuf *m_split(struct mbuf *, int, int);
struct mbuf *m_uiotombuf(struct uio *, int, int, int, int);
-struct mbuf *m_unshare(struct mbuf *, int how);
+struct mbuf *m_unshare(struct mbuf *, int);
/*-
* Network packets may have annotations attached by affixing a list of
Index: kern/uipc_mbuf.c
===================================================================
--- kern/uipc_mbuf.c (revision 245450)
+++ kern/uipc_mbuf.c (working copy)
@@ -85,6 +85,79 @@
#endif
/*
+ * m_get2() allocates minimum mbuf that would fit "size" argument.
+ */
+struct mbuf *
+m_get2(int how, short type, int flags, int size)
+{
+ struct mb_args args;
+ struct mbuf *m, *n;
+ uma_zone_t zone;
+
+ args.flags = flags;
+ args.type = type;
+
+ if (size <= MHLEN || (size <= MLEN && (flags & M_PKTHDR) == 0))
+ return (uma_zalloc_arg(zone_mbuf, &args, how));
+ if (size <= MCLBYTES)
+ return (uma_zalloc_arg(zone_pack, &args, how));
+ if (size > MJUM16BYTES)
+ return (NULL);
+
+ m = uma_zalloc_arg(zone_mbuf, &args, how);
+ if (m == NULL)
+ return (NULL);
+
+#if MJUMPAGESIZE != MCLBYTES
+ if (size <= MJUMPAGESIZE)
+ zone = zone_jumbop;
+ else
+#endif
+ if (size <= MJUM9BYTES)
+ zone = zone_jumbo9;
+ else
+ zone = zone_jumbo16;
+
+ n = uma_zalloc_arg(zone, m, how);
+ if (n == NULL) {
+ uma_zfree(zone_mbuf, m);
+ return (NULL);
+ }
+
+ return (m);
+}
+
+/*
+ * m_getjcl() returns an mbuf with a cluster of the specified size attached.
+ * For size it takes MCLBYTES, MJUMPAGESIZE, MJUM9BYTES, MJUM16BYTES.
+ */
+struct mbuf *
+m_getjcl(int how, short type, int flags, int size)
+{
+ struct mb_args args;
+ struct mbuf *m, *n;
+ uma_zone_t zone;
+
+ if (size == MCLBYTES)
+ return m_getcl(how, type, flags);
+
+ args.flags = flags;
+ args.type = type;
+
+ m = uma_zalloc_arg(zone_mbuf, &args, how);
+ if (m == NULL)
+ return (NULL);
+
+ zone = m_getzone(size);
+ n = uma_zalloc_arg(zone, m, how);
+ if (n == NULL) {
+ uma_zfree(zone_mbuf, m);
+ return (NULL);
+ }
+ return (m);
+}
+
+/*
* Allocate a given length worth of mbufs and/or clusters (whatever fits
* best) and return a pointer to the top of the allocated chain. If an
* existing mbuf chain is provided, then we will append the new chain
--BwCQnh7xodEAoBMC--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130115135617.GJ79056>