kern/156180
Karim Fodil-Lemelin
fodillemlinkarim at gmail.com
Wed Apr 6 18:20:13 UTC 2011
The following reply was made to PR kern/156180; it has been noted by GNATS.
From: Karim Fodil-Lemelin <fodillemlinkarim at gmail.com>
To: Gleb Smirnoff <glebius at FreeBSD.org>
Cc:
Subject: Re: kern/156180
Date: Wed, 6 Apr 2011 14:07:08 -0400
--000e0cd5d1826dc62504a043dcf6
Content-Type: text/plain; charset=ISO-8859-1
Hi Gleb,
2011/4/6 Gleb Smirnoff <glebius at freebsd.org>
> Hi, Karim!
>
> On Wed, Apr 06, 2011 at 10:36:46AM -0400, Karim Fodil-Lemelin wrote:
> K> Thanks for the patch it does work in FBSD although it does not work in
> my
> K> setup since I have extended TCP option checking into another ipfw action
> and
> K> while I could add the check you've proposed for tcpop_match I would
> prefer a
> K> more generic approach where the m_pullup call is done for all TCP
> packets
> K> with options (basically in the case IPPROTO_TCP).
> K>
> K> The rationale behind this is such that there is a guarantee that
> tcpop_match
> K> will work but also that any future extensions based on TCP options would
> K> also work saving the hard to debug situation that a missing call to
> m_pullup
> K> can create.
>
> Currently almost all TCP traffic has TCP options. And currently most,
> I suppose > 90%, installations, that use ipfw(4) do not have rules with
> 'tcpoptions' keyword. So, we would add extra pullup, that is not needed
> in most cases and may have a performance impact.
>
In my case even if all packets have at least a timestamp option or SACK the
actual m_pullup was only called once every 10s of thousands packet as most
of the time the lower layers hands out perfectly contiguous headers.
What I've found is whenever a netgraph node or some other decapsulation
mechanisms that tends to modify headers and due to its internal operations
needs to somewhat break down the mbuf chain, the m_pullup call might be
needed due to trailing options being in another mbuf. Most other times the
headers are just fine and thats what made this problem hard to find in the
first place.
So I don't think there is a significant performance hit but I do see your
point (see my point on multiple TCP microinstructions below).
> In case of future extensions the hard to debug situation won't happen
> if a developer analyses the function he modifies thoroughly.
>
One could assume that adding an action to ipfw should be relatively
straightforward and the mechanics of doing sanity checks and what not was
already handled by the top half of the function. Although I'm sure that with
the call to PULLUP_LEN in tcpopt_match developers basing their code on it
will not miss it ;)
I can understand that since TCP option filtering isn't used much by the
community its better to keep its specific processing isolated from IP
although that approach has its drawback. You can imagine that if multiple
TCP actions are chained (as microinstructions) together (and so are the
calls to PULLUP_LEN) the call overhead becomes quickly a performance issue.
>
> So, can you please confirm, that if you adding this string
>
> PULLUP_LEN(hlen, ulp, (TCP(ulp)->th_off << 2));
>
> into your new ipfw action, solves the discussed problem?
>
Unfortunately I won't be able to test this exact code, as I mentionned
earlier, since it is not applicable to our solution (btw the problem would
show up once every 2 days or so). My PR was really just a mere pointer to an
issue I have encountered and I perfectly understand the context of your
solution and its applicability. You see myself in the impossibility to
confirm with testing your proposed patch.
If that can be a consolation my first fix was to add a call to m_pullup in
every TCP action we had and it worked just fine although the final solution
was to integrate it higher up in the function (before the microinstruction
loop). Though, I do have to admit I prefer your macro integration to what
I've proposed since it 'blends in' better with the previous implementation.
Best regards,
Karim.
--000e0cd5d1826dc62504a043dcf6
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Hi Gleb,<br><br><div class=3D"gmail_quote">2011/4/6 Gleb Smirnoff <span dir=
=3D"ltr"><<a href=3D"mailto:glebius at freebsd.org">glebius at freebsd.org</a>=
></span><br><blockquote class=3D"gmail_quote" style=3D"border-left: 1px =
solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
=A0Hi, Karim!<br>
<br>
On Wed, Apr 06, 2011 at 10:36:46AM -0400, Karim Fodil-Lemelin wrote:<br>
K> Thanks for the patch it does work in FBSD although it does not work i=
n my<br>
K> setup since I have extended TCP option checking into another ipfw act=
ion and<br>
K> while I could add the check you've proposed for tcpop_match I wou=
ld prefer a<br>
K> more generic approach where the m_pullup call is done for all TCP pac=
kets<br>
K> with options (basically in the case IPPROTO_TCP).<br>
K><br>
K> The rationale behind this is such that there is a guarantee that tcpo=
p_match<br>
K> will work but also that any future extensions based on TCP options wo=
uld<br>
K> also work saving the hard to debug situation that a missing call to m=
_pullup<br>
K> can create.<br>
<br>
Currently almost all TCP traffic has TCP options. And currently most,<br>
I suppose > 90%, installations, that use ipfw(4) do not have rules with<=
br>
'tcpoptions' keyword. So, we would add extra pullup, that is not ne=
eded<br>
in most cases and may have a performance impact.<br></blockquote><div><br>I=
n my case even if all packets have at least a timestamp option or SACK the =
actual m_pullup was only called once every 10s of thousands packet as most =
of the time the lower layers hands out perfectly contiguous headers.<br>
<br>What I've found is whenever a netgraph node or some other decapsula=
tion mechanisms that tends to modify headers and due to its internal operat=
ions needs to somewhat break down the mbuf chain, the m_pullup call might b=
e needed due to trailing options being in another mbuf. Most other times th=
e headers are just fine and thats what made this problem hard to find in th=
e first place.<br>
<br>So I don't think there is a significant performance hit but I do se=
e your point (see my point on multiple TCP microinstructions below).<br><br=
></div><blockquote class=3D"gmail_quote" style=3D"border-left: 1px solid rg=
b(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
In case of future extensions the hard to debug situation won't happen<b=
r>
if a developer analyses the function he modifies thoroughly.<br></blockquot=
e><div>=A0</div><div>One could assume that adding an action to ipfw should =
be relatively straightforward and the mechanics of doing sanity checks and =
what not was already handled by the top half of the function. Although I=
9;m sure that with the call to PULLUP_LEN in tcpopt_match developers basing=
their code on it will not miss it ;)<br>
<br>I can understand that since TCP option filtering isn't used much by=
the community its better to keep its specific processing isolated from IP =
although that approach has its drawback. You can imagine that if multiple T=
CP actions are chained (as microinstructions) together (and so are the call=
s to PULLUP_LEN) the call overhead becomes quickly a performance issue.<br>
=A0</div><blockquote class=3D"gmail_quote" style=3D"border-left: 1px solid =
rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
So, can you please confirm, that if you adding this string<br>
<br>
PULLUP_LEN(hlen, ulp, (TCP(ulp)->th_off << 2));<br>
<br>
into your new ipfw action, solves the discussed problem?<br></blockquote></=
div><br>Unfortunately I won't be able to test this exact code, as I men=
tionned earlier, since it is not applicable to our solution (btw the proble=
m would show up once every 2 days or so). My PR was really just a mere poin=
ter to an issue I have encountered and I perfectly understand the context o=
f your solution and its applicability. You see myself in the impossibility =
to confirm with testing your proposed patch.<br>
<br>If that can be a consolation my first fix was to add a call to m_pullup=
in every TCP action we had and it worked just fine although the final solu=
tion was to integrate it higher up in the function (before the microinstruc=
tion loop). Though, I do have to admit I prefer your macro integration to w=
hat I've proposed since it 'blends in' better with the previous=
implementation.<br>
<br>Best regards,<br><br>Karim.<br>
--000e0cd5d1826dc62504a043dcf6--
More information about the freebsd-ipfw
mailing list