m_devget() and const buffer
Hi. Does anybody knows why m_devget() receives non-const char* as first argument? Is there any destructive copy routines used with it? I have searched all the kernel, but haven't found any. May be we could change that? -- Alexander Motin ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
Re: HEADS UP: vimage - virtualized global variables in the network stack
From: Max Laier Subject: Re: HEADS UP: vimage - virtualized global variables in the network stack Date: Sat, 13 Dec 2008 20:45:16 +0100 > On Saturday 13 December 2008 20:33:53 Bjoern A. Zeeb wrote: > ... > > This state of having the variables in parallel, global and in the > > container struct, will be maintained for another (short) time until > > the entire virtualization framework is in. This is needed, so that > > all three possible states can be benchmarked from exactly the same > > code changeset. > > > > > > For developers comitting new code or changing code it is important to > > properly add virtualized variables in the way that: > > 1) the globals and externs (if needed) are added/kept in sync as both > > a) globals under #ifdef VIMAGE_GLOBALS and b) to the appropriate > > container struct + the V_ macro. > > When used somewhere in code one has to use the V_foobarbaz version. > > Is there (an easy) way to have the tinderbox build every other run without > VIMAGE_GLOBALS so that the most obvious error (global available, but not in > the container struct - or the other way around) can be warned about? This actually points out why the 'tinderbox' name is bogus for the universe plus failure: universe builds all the kernels. Tinderbox builds LINT only. Warner ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
Re: HEADS UP: vimage - virtualized global variables in the network stack
On Sat, 13 Dec 2008, Max Laier wrote: On Saturday 13 December 2008 20:33:53 Bjoern A. Zeeb wrote: ... This state of having the variables in parallel, global and in the container struct, will be maintained for another (short) time until the entire virtualization framework is in. This is needed, so that all three possible states can be benchmarked from exactly the same code changeset. For developers comitting new code or changing code it is important to properly add virtualized variables in the way that: 1) the globals and externs (if needed) are added/kept in sync as both a) globals under #ifdef VIMAGE_GLOBALS and b) to the appropriate container struct + the V_ macro. When used somewhere in code one has to use the V_foobarbaz version. Is there (an easy) way to have the tinderbox build every other run without VIMAGE_GLOBALS so that the most obvious error (global available, but not in the container struct - or the other way around) can be warned about? Without VIMAGE_GLOBALS is the default; we have been building this for a few days already. The flip had been so smoothly that almost noone had really noticed. Marko has done a really great job! Thus my HEADS UP now after I am confident enough that (almost) all places were caught and clean. In case you want to check yourself you can simply put a file into one or multiple archs conf dir that looks like: cat sys/amd64/conf/LINT-VIMAGE_GLOBALS include LINT ident LINT-VIMAGE_GLOBALS options VIMAGE_GLOBALS I am doing that build every other day from now to catch the possible error of a virtualized variable in the container struct w/o the global. But as this is the least problematic case I do not want to commit the kernel configuration as it'll make builds longer for everyone, etc. The more problematic cases that builds cannot catch are: - static initialization of a virtualized 'global'. - a newly added virtualized 'global' that is not under #ifdef VIMAGE_GLOBALS I have scripts to identify the latter already. The former will only be caught be either code inspection or "unexpected results" when running. /bz -- Bjoern A. Zeeb The greatest risk is not taking one. ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
HEADS UP: vimage - virtualized global variables in the network stack
On Sat, 13 Dec 2008, Bjoern A. Zeeb wrote:
Hi,
Author: bz
Date: Sat Dec 13 19:13:03 2008
New Revision: 186048
URL: http://svn.freebsd.org/changeset/base/186048
Log:
Second round of putting global variables, which were virtualized
but formerly missed under VIMAGE_GLOBAL.
Put the extern declarations of the virtualized globals
under VIMAGE_GLOBAL as the globals themsevles are already.
This will help by the time when we are going to remove the globals
entirely.
As some of you might have noticed that Marko's last commit for the
first time in the series of vimage commits was an actual functional
change. By default HEAD is no longer using the globals.
With my commit the current set of virtualized variables is assumed to
be "clean". This basically means three things:
1) The former globals and their externs are all under #ifdef VIMAGE_GLOBALS
2) The same variables are present in a 'container struct'
3) The initialization of those is done from 'constructor ("init")
functions'
This state of having the variables in parallel, global and in the
container struct, will be maintained for another (short) time until
the entire virtualization framework is in. This is needed, so that
all three possible states can be benchmarked from exactly the same
code changeset.
For developers comitting new code or changing code it is important to
properly add virtualized variables in the way that:
1) the globals and externs (if needed) are added/kept in sync as both
a) globals under #ifdef VIMAGE_GLOBALS and b) to the appropriate
container struct + the V_ macro.
When used somewhere in code one has to use the V_foobarbaz version.
2) Any new virtualized globals must not be directly initialized.
They have to be initialized from a contructor function (which is
usually there already).
If you are confused about some of the terms etc. follow the links in
the "Some documentation" section on the wiki:
http://wiki.freebsd.org/Image
The "Vimage Coding - beginners guide / FAQ" tries to answer the 101
questions. For the beaf you'll find the link to a document in perforce
with the last question (that you may already know).
We'll try to enhance this as we get questions or the integration goes
on.
In case of questions or suggestions ideally follow-up
on freebsd-virtualization@ .
/bz
--
Bjoern A. Zeeb The greatest risk is not taking one.
___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
Re: HEADS UP: vimage - virtualized global variables in the network stack
On Saturday 13 December 2008 20:33:53 Bjoern A. Zeeb wrote: ... > This state of having the variables in parallel, global and in the > container struct, will be maintained for another (short) time until > the entire virtualization framework is in. This is needed, so that > all three possible states can be benchmarked from exactly the same > code changeset. > > > For developers comitting new code or changing code it is important to > properly add virtualized variables in the way that: > 1) the globals and externs (if needed) are added/kept in sync as both > a) globals under #ifdef VIMAGE_GLOBALS and b) to the appropriate > container struct + the V_ macro. > When used somewhere in code one has to use the V_foobarbaz version. Is there (an easy) way to have the tinderbox build every other run without VIMAGE_GLOBALS so that the most obvious error (global available, but not in the container struct - or the other way around) can be warned about? -- /"\ Best regards, | mla...@freebsd.org \ / Max Laier | ICQ #67774661 X http://pf4freebsd.love2party.net/ | mla...@efnet / \ ASCII Ribbon Campaign | Against HTML Mail and News ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
Re: Heads up --- Thinking about UDP and tunneling
On Friday 12 December 2008 14:46:30 Randall Stewart wrote:
> Ok:
>
> Here is an updated patch it:
>
> 1) Fixes style9 issues
> 2) move to _t typedef
I won't get into this.
> 3) Allow multicast/broadcast to also be tunneled.
There seems to be an error with your patch. You RUNLOCK twice in some places
- should be fixed in my version of the patch (see attached).
> 4) Binding is now no longer a requirement to set tunneling mode, in
> fact most protocols had better NOT have bound.. first set options, then
> bind :-)
Yep.
> There was one thing I was a bit leary of for <3>. In the loop for
> going through all the inp's of UDP that are listening to a m-cast/b-cast
> I could not release the INP_INFO_LOCK() in other cases I make sure all
> locks are released when we call into the tunneling protocol. I think
> this will be ok as long as the tunnelee does not try to use the
> INP_INFO_LOCK of the UDP world... I know for SCTP this is a non-issue.. but
> it may be something we want to think about... not sure.
I added a comment inline. Maybe we should add a flag to the tunnel function
prototype, so that the called party can figure out if they need to defer work
that is not allowed under the lock (e.g. malloc WAITOK).
On top of that, I was wondering if it might make sense to pass in the inp as
an argument - so that the tunnel consumer can figure out which tunnel the
packet was received on (without relying on data off the wire). In that case
I'd leave the inp RLOCK'ed and leave it up the the called party to release the
lock if they so desire.
> If there are no serious objections I will submit this into head.. and
There are some open issues, see above - but in general okay with me.
> followed behind it I will send in the changes so SCTP can be tunneled over
> this new mechanism :-)
--
/"\ Best regards, | mla...@freebsd.org
\ / Max Laier | ICQ #67774661
X http://pf4freebsd.love2party.net/ | mla...@efnet
/ \ ASCII Ribbon Campaign | Against HTML Mail and News
Index: netinet/udp_var.h
===
--- netinet/udp_var.h (revision 186046)
+++ netinet/udp_var.h (working copy)
@@ -110,6 +110,10 @@
voidudp_input(struct mbuf *, int);
struct inpcb *udp_notify(struct inpcb *inp, int errno);
int udp_shutdown(struct socket *so);
+
+
+typedef void(*udp_tunnel_function_t)(struct mbuf *, int off);
+int udp_set_kernel_tunneling(struct socket *so, udp_tunnel_function_t f);
#endif
#endif
Index: netinet/udp_usrreq.c
===
--- netinet/udp_usrreq.c(revision 186046)
+++ netinet/udp_usrreq.c(working copy)
@@ -488,10 +488,29 @@
struct mbuf *n;
n = m_copy(m, 0, M_COPYALL);
- if (n != NULL)
- udp_append(last, ip, n, iphlen +
- sizeof(struct udphdr), &udp_in);
- INP_RUNLOCK(last);
+
+ if (last->inp_ppcb == NULL) {
+ if (n != NULL)
+ udp_append(last, ip, n, iphlen +
+ sizeof(struct udphdr),
&udp_in);
+ INP_RUNLOCK(last);
+ } else {
+ /* Engage the tunneling protocol
+* we will have to leave the info_lock
+* up, since we are hunting through
+* multiple UDP inp's hope we don't
break :-(
+*
+* XXXML: Maybe add a flag to the
+* prototype so that the tunneling
+* can defer work that can't be done
+* under the info lock?
+*/
+ udp_tunnel_function_t tunnel_func;
+
+ tunnel_func =
(udp_tunnel_function_t)last->inp_ppcb;
+ INP_RUNLOCK(last);
+ tunnel_func(m, iphlen);
+ }
}
last = inp;
/*
@@ -516,10 +535,24 @@
V_udpstat.udps_noportbcast++;
goto badheadlocked;
}
- udp_append(last, ip, m, iphlen + sizeof(struct udphdr),
- &udp_in);
- INP_RUNLOCK(last);
- INP_INFO_RUNLOCK(&V_udbinfo);
+ if (last->inp_ppc
Re: Problem with new source address selection
Bjoern A. Zeeb wrote on 8 Dec 2008 21:02: > Did you try the patch and did it work for you as expected? If so I'll > add it to my repo and the next jail patch. Meanwhile I can confirm that your patch works well for me on an up-to- date RELENG_7 kernel. Thanks! Frank -- Frank Behrens, Osterwieck, Germany PGP-key 0x5B7C47ED on public servers available. ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
Re: Heads up --- Thinking about UDP and tunneling
On Fri, 12 Dec 2008, Randall Stewart wrote: 1) I went ahead and fixed the comments.. even added a ! instead of :-( 2) No problem using func_t.. changed to that.. seems nicer :-D 3) Removed an extra cr or two you pointed out.. hopefully got them all. OK. 4) I disagree with you on the cast... its not ugly.. it prevents us from having to have a per_pcb structure for UDP when all we need is one pointer. As I said in my first post.. it seemed like to much overhead, creating a zone for a single pointer... I am not adverse to casts .. but of course I guess I am just an old fart who has been around to many years writing code :-) This is actually more broken than I first thought. inp_ppcb is "void *" but is abused to hold a function pointer. This gives undefined behaviour with or without the cast. The actual behaviour is apparently to work on all supported machines, but to suppress printing of a diagnostic with the cast. On ia64, function pointers are very different from "void *", but they can still be represented as "void *" after a conversion. I think ia64 function pointers are implemented as something like an integer index into a table of the actual pointers. Since 2^32 function pointers should be enough for anyone, 64-bit "void *" is more than large enough to represent them. If inp_ppcb were a generic function pointer, then a conversion would still be needed, but it doesn't need to be a cast. Simple assignment without the cast must work. However, I think we have warning flags set to such a high level that there would be warnings for the type change done by assignment, and the cast would be needed anyway to suppress the warning. I don't like this. Casts normally do suppress warnings and this hides errors too. Of course, saving space is a valid reason for this hack. 5) I ran s9indent.. and of course it found a lot of other things you missed.. but that is the way of s9indent I have found :-D Not surprising that it has bugs :-). Apart from the types of errors mentioned in my previous mail, it does the following unformatting: [hard carriage returns removed from diff] % Index: netinet/udp_usrreq.c % === % --- netinet/udp_usrreq.c (revision 185928) % +++ netinet/udp_usrreq.c (working copy) % @@ -97,7 +97,8 @@ % */ % % #ifdef VIMAGE_GLOBALS % -int udp_blackhole; % +int udp_blackhole; % + % #endif Hmm, it mangles #endif the same as indent(1), so it seems to be just a wrapper for indent(1), just with slightly worse directives than my knfometer. [% ...] I don't notice many more problems than mentioned in my previous mail. The slightly worse directives are -c33 instead of -c41 and whatever directive it is that breaks the tab indentation for udp_blackhole above. BTW, I used to use that directive in knfometer too. It was needed because most declarations are local, but old FreeBSD indent and gnu indent don't support different formatting for local declarations, so the directive that gave the least mangling was the one that preserved the formatting of local declarations. I eventually modified FreeBSD indent to support the different formatting. Not having this is one of the main problems that makes gnu indent not directly usable for formatting to KNF (gnu indent is generally better but has fewer features -- surprising for a gnu utility). % @@ -387,7 +395,8 @@ % uh->uh_dport = ntohs(next_hop->sin_port); % % /* % - * Remove the tag from the packet. We don't need it anymore. % + * Remove the tag from the packet. We don't need it % + * anymore. %*/ % m_tag_delete(m, fwd_tag); % } No need to change this. Another bug in indent is that it has no flexibility for preserving formatting that is good enough. Normally you don't want large block comments reformatted. The line length parameter only works for reformatting comments, but minor changes in it normally lead to complete reformatting of block comments. knfometer uses directives to suppress reformatting of all block comments although this misses some necessary reformatting. % @@ -414,10 +423,11 @@ % inp->inp_faddr.s_addr != ip->ip_src.s_addr) % continue; % /* % - * XXX: Do not check source port of incoming datagram % - * unless inp_connect() has been called to bind the % - * fport part of the 4-tuple; the source could be % - * trying to talk to us with an ephemeral port. % + * XXX: Do not check source port of incoming % + * datagram unless inp_connect() has been called to % + * bind the fport part of the 4-tuple; the source % + * could be trying to talk to us with an ephemeral % + * port. %
Re: Heads up --- Thinking about UDP and tunneling
On Fri, 12 Dec 2008, Randall Stewart wrote: Well tell you what Bruce: How about if I just run the WHOLE file through s9indent... This will fix ALL my problems.. and of course "fix" the rest of the file too.. Any automated conversion utility is very likely to introduce more bugs than it fixes. I haven't seen any that even try to work right. You would have to inspect every change and throw out most. With a working-right utility, you wouldn't have to do this since you would direct it to make only changes that you really want, including somehow restricting the changes to your new code. Unless you think its already in conformance (bet you its not) :-) According to my (buggy) knfometer utilty (just indent(1) with certain options followed by comparision of the size of the diff with the size of the original file): 94.325% udp_usrreq.c 80.452% udp_var.h 93.528% udp6_usrreq.c Anything above 90% is good and indicates that most of the changes are due to bugs in the conversion utility. In fact, indent seems to be making much the same wrong changes as your s9indent (I see the patch from that in a later reply -- I might respond to that too). Here is what indent does with udp_usrreq.c (the diff is larger than 5.675% of the file since it is -u2 while 5.675% is for a plain diff): % --- udp_usrreq.c.BAK 2008-12-13 09:49:18.39864 + % +++ udp_usrreq.c 2008-12-13 09:49:18.40364 + % @@ -99,4 +99,5 @@ % #ifdef VIMAGE_GLOBALS % int udp_blackhole; % + % #endif % Bug in indent. indent knows that it doesn't understand cpp directives so it tries not to touch them, but it still botches #endif by adding a blank line (and by misformatting any comment on #endif). % @@ -107,9 +108,11 @@ % * cause problems (especially for NFS data blocks). % */ % -static int udp_cksum = 1; % +static int udp_cksum = 1; Bug in indent, same as in s9indent. indent doesn't understand the arcane rule followed by the original code here, though I have directed it to indent by 1 tab after declaration-specificers. The problem is that indent knows that there are problems doing this for declaration-specifers longer than 7 characters (since the tab couldn't give the normal indent of 8 characters then), while the above code is happy to indent all the variable names to 16 characters. % + Bug in indent. Not shared by s9indent. The problem is that indent doesn't understand the ugly SYSCTL_INT() in the next statement. It thinks that SYSCTL_INT() is a function definition and has been directed to put a blank line before function definitions. % SYSCTL_INT(_net_inet_udp, UDPCTL_CHECKSUM, checksum, CTLFLAG_RW, &udp_cksum, % 0, "compute udp checksum"); % % int udp_log_in_vain = 0; % + Same bug as above. Now the indentation is unchanged though it is inconsistent with previous indentation, since the declaration-specifier is short. s9indent breaks the indentation here, since it doesn't understand or hasn't been directed that variable names should be indented by a tab (except in auto declarations). % SYSCTL_INT(_net_inet_udp, OID_AUTO, log_in_vain, CTLFLAG_RW, % &udp_log_in_vain, 0, "Log all incoming UDP packets"); % @@ -120,5 +123,6 @@ % % u_long udp_sendspace = 9216; /* really max datagram size */ % - /* 40 1K datagrams */ % + % + /* 40 1K datagrams */ Bug in indent, same as in s9indent. indent doesn't understand that the second comment is part of the first (which is a hard thing to understand), so it makes a mess. % SYSCTL_ULONG(_net_inet_udp, UDPCTL_MAXDGRAM, maxdgram, CTLFLAG_RW, % &udp_sendspace, 0, "Maximum outgoing UDP datagram size"); % @@ -126,9 +130,9 @@ % u_long udp_recvspace = 40 * (1024 + % #ifdef INET6 % - sizeof(struct sockaddr_in6) % +sizeof(struct sockaddr_in6) % #else % - sizeof(struct sockaddr_in) % +sizeof(struct sockaddr_in) % #endif % - ); % +); indent mangles the special formatting here, the same as s9indent. It is following the strict rules here (except the final ");" should be indented by 4 characters too), but the original code has fancy formatting that intentionally breaks the rules. The strict rules are also wrong. indent doesn't really understand initializers in declarations. For function calls, it would start the 4-character continuation indent at the start of the function name, but it doesn't have a corresponding rule for initalizers and indents relative to column 0. % % SYSCTL_ULONG(_net_inet_udp, UDPCTL_RECVSPACE, recvspace, CTLFLAG_RW, % @@ -136,7 +140,8 @@ % % #ifdef VIMAGE_GLOBALS % -struct inpcbhead udb;/* from udp_var.h */ % -struct inpcbinfo udbinfo; % -struct udpstat udpstat;/* from udp_var.h */ % +struct inpcbhead udb;/* from udp_var.h */ % +struct inpcbinfo udbinfo; % +struct udpstat udpstat; /* f
Re: Heads up --- Thinking about UDP and tunneling
Ian: No problem what so ever... My native style is CLOSE to style(9).. but it often is hard for me to get "all" the little twists. And with SCTP we have 2 other primary developers and a few other contributors that work on the windoz stuff and user space... so we depend on s9indent exclusively.. Being an old-fart its hard to change style... and when emacs does not help me it makes it even worse ;-0 I have noticed over the last few years my style has been evolving closer and closer to style9.. but after 28 years of writing kernel level C code style changes slowly :-0 (old dogs and all that). Actually unless you are looking for excess blank lines, missing "."'s in comments and minor nits... I think I am pretty close to style9.. not that I am perfect (thats why s9indent is a plus ;-D)... I try.. but like all of us I am not perfect by a LONG LONG way (just ask my wife :-D) I will let this hang here for a while.. I have to head up for U of Del for a PHd committee I am on... so I will see if over the next week or so anyone else throws flames out... (I am more concerned about the functioning of the code then the style actually.. but hey I understand some folks are "style nazi's" thats ok... thats probably why s9indent was invented ;-D) R On Dec 13, 2008, at 4:05 AM, Ian Smith wrote: On Sat, 13 Dec 2008, Peter Jeremy wrote: On 2008-Dec-13 13:55:18 +1100, Ian Smith wrote: I guess submitting patches for style(9) is considered a suicide method? Not necessarily but you need to have very good justification for any change. It's much easier to read a large corpus of code where the code is all written in one style. I suspect that no-one is happy with everything in style(9) but consistency is seen as more important. No doubt. Apologies to all, especially Randall, for a flippant and off-topic post. cheers, Ian ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org" -- Randall Stewart 803-317-4952 (cell) 803-345-0391(direct) ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
Re: Heads up --- Thinking about UDP and tunneling
On Sat, 13 Dec 2008, Peter Jeremy wrote: > On 2008-Dec-13 13:55:18 +1100, Ian Smith wrote: > >I guess submitting patches for style(9) is considered a suicide method? > > Not necessarily but you need to have very good justification for any > change. It's much easier to read a large corpus of code where the > code is all written in one style. I suspect that no-one is happy > with everything in style(9) but consistency is seen as more important. No doubt. Apologies to all, especially Randall, for a flippant and off-topic post. cheers, Ian ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
