don't clone skb when skb isn't shared
When the tcf_action is TC_ACT_STOLEN, and the skb isn't shared, we don't need
to clone a new skb. As the skb will be freed after this function returns, we
can use it freely once we get a reference to it.
Signed-off-by: Changli Gao <redacted>
----
net/sched/act_mirred.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
On Fri, 2010-06-04 at 21:43 +0800, Changli Gao wrote:
don't clone skb when skb isn't shared
When the tcf_action is TC_ACT_STOLEN, and the skb isn't shared, we don't need
to clone a new skb. As the skb will be freed after this function returns, we
can use it freely once we get a reference to it.
It looks like a good optimization - but i am not a big fan of one-offs
[because usability goes down and I am forced to explain it longer in the
rules (refer to: Documentation/networking/tc-actions-env-rules.txt)]
How about you update skb_act_clone to take take the action code as well
and do the check the if stolen/queued it does a skb_get otherwise it
calls skb_clone?
cheers,
jamal
On Sat, Jun 5, 2010 at 8:53 PM, jamal [off-list ref] wrote:
On Fri, 2010-06-04 at 21:43 +0800, Changli Gao wrote:
quoted
don't clone skb when skb isn't shared
When the tcf_action is TC_ACT_STOLEN, and the skb isn't shared, we don't need
to clone a new skb. As the skb will be freed after this function returns, we
can use it freely once we get a reference to it.
It looks like a good optimization - but i am not a big fan of one-offs
[because usability goes down and I am forced to explain it longer in the
rules (refer to: Documentation/networking/tc-actions-env-rules.txt)]
Thanks. BTW: act_nat.c doesn't obey the following rule, and you plan
to remove TC_MUNGED and TC_OK2MUNGE?
2) If you munge any packet thou shalt call pskb_expand_head in the case
someone else is referencing the skb. After that you "own" the skb.
You must also tell us if it is ok to munge the packet (TC_OK2MUNGE),
this way any action downstream can stomp on the packet.
How about you update skb_act_clone to take take the action code as well
and do the check the if stolen/queued it does a skb_get otherwise it
calls skb_clone?
Good idea. Thanks.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
On Sat, 2010-06-05 at 21:07 +0800, Changli Gao wrote:
Thanks. BTW: act_nat.c doesn't obey the following rule, and you plan
to remove TC_MUNGED and TC_OK2MUNGE?
2) If you munge any packet thou shalt call pskb_expand_head in the case
someone else is referencing the skb. After that you "own" the skb.
You must also tell us if it is ok to munge the packet (TC_OK2MUNGE),
this way any action downstream can stomp on the packet.
That rule still applies but it is upto the discretion of the action.
i.e if the act_nat thinks it is ok for others down the street to trample
on the packet, it should tell us so. Maybe i should change the wording
to use the word "may" in that 3rd sentence.
[I will kill (low prio) TC_OK2MUNGE but not TC_MUNGED.]
cheers,
jamal
On Sat, Jun 5, 2010 at 9:24 PM, jamal [off-list ref] wrote:
On Sat, 2010-06-05 at 21:07 +0800, Changli Gao wrote:
quoted
Thanks. BTW: act_nat.c doesn't obey the following rule, and you plan
to remove TC_MUNGED and TC_OK2MUNGE?
quoted
2) If you munge any packet thou shalt call pskb_expand_head in the case
someone else is referencing the skb. After that you "own" the skb.
You must also tell us if it is ok to munge the packet (TC_OK2MUNGE),
this way any action downstream can stomp on the packet.
That rule still applies but it is upto the discretion of the action.
i.e if the act_nat thinks it is ok for others down the street to trample
on the packet, it should tell us so. Maybe i should change the wording
to use the word "may" in that 3rd sentence.
[I will kill (low prio) TC_OK2MUNGE but not TC_MUNGED.]
If you kill TC_OK2MUNGE, you should kill TC_MUNGED too, as it will
become useless.
localhost linux # grep MUNGE net/sched/ -R
net/sched/act_pedit.c: if (!(skb->tc_verd & TC_OK2MUNGE)) {
net/sched/act_pedit.c: skb->tc_verd =
SET_TC_MUNGED(skb->tc_verd);
net/sched/act_api.c: if (TC_MUNGED & skb->tc_verd) {
net/sched/act_api.c: skb->tc_verd =
SET_TC_OK2MUNGE(skb->tc_verd);
net/sched/act_api.c: skb->tc_verd =
CLR_TC_MUNGED(skb->tc_verd);
int tcf_action_exec(struct sk_buff *skb, struct tc_action *act,
struct tcf_result *res)
{
struct tc_action *a;
int ret = -1;
if (skb->tc_verd & TC_NCLS) {
skb->tc_verd = CLR_TC_NCLS(skb->tc_verd);
ret = TC_ACT_OK;
goto exec_done;
}
while ((a = act) != NULL) {
repeat:
if (a->ops && a->ops->act) {
ret = a->ops->act(skb, a, res);
if (TC_MUNGED & skb->tc_verd) {
/* copied already, allow trampling */
skb->tc_verd = SET_TC_OK2MUNGE(skb->tc_verd);
skb->tc_verd = CLR_TC_MUNGED(skb->tc_verd);
}
if (ret == TC_ACT_REPEAT)
goto repeat; /* we need a ttl - JHS */
if (ret != TC_ACT_PIPE)
goto exec_done;
}
act = a->next;
}
exec_done:
return ret;
}
The bit OK2MUNGE relies on MUNGED only.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
On Sat, 2010-06-05 at 21:33 +0800, Changli Gao wrote:
If you kill TC_OK2MUNGE, you should kill TC_MUNGED too, as it will
become useless.
Those two have different semantics and use different verdict bits:
one is saying "this packet has been munged" another is "I give you ok to
munge this packet".
Thanks for the suggestion - I will think about it some more before
deleting anything.
cheers,
jamal