| Message ID | 1279860845-7177-1-git-send-email-xiaosuo@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
Le vendredi 23 juillet 2010 à 12:54 +0800, Changli Gao a écrit : > This patch should be applied after my another patch: > http://patchwork.ozlabs.org/patch/59729/ > > xt_quota: don't copy quota back to userspace > > In nowadays, table entries are per-cpu variables, so it don't make any sense to > copy quota back to one of the variable instances. To keep things simple, this > patch undo the copy. > > Signed-off-by: Changli Gao <xiaosuo@gmail.com> This looks the wrong way to fix this problem. Also Changli, could you please _not_ include the title of your patches inside the Changelog ? This is useless. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le vendredi 23 juillet 2010 à 07:27 +0200, Eric Dumazet a écrit : > Le vendredi 23 juillet 2010 à 12:54 +0800, Changli Gao a écrit : > > This patch should be applied after my another patch: > > http://patchwork.ozlabs.org/patch/59729/ > > > > xt_quota: don't copy quota back to userspace > > > > In nowadays, table entries are per-cpu variables, so it don't make any sense to > > copy quota back to one of the variable instances. To keep things simple, this > > patch undo the copy. > > > > Signed-off-by: Changli Gao <xiaosuo@gmail.com> > > This looks the wrong way to fix this problem. > > Also Changli, could you please _not_ include the title of your patches > inside the Changelog ? This is useless. > Reading again your patch, I understand only the Changelog is wrong. We want to copy quota back to userspace, as specified when rule was setup (so that iptables-save works) The real thing you are doing is that we dont change the initial quota during packet processing, only the private quota, shared by all cpus. Before the patch , iptables -nvL could report an old and not accurate quota value. After the patch, iptables -nvL reports the initial quota value, not the actual value. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 23, 2010 at 1:27 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le vendredi 23 juillet 2010 à 12:54 +0800, Changli Gao a écrit : >> This patch should be applied after my another patch: >> http://patchwork.ozlabs.org/patch/59729/ >> >> xt_quota: don't copy quota back to userspace >> >> In nowadays, table entries are per-cpu variables, so it don't make any sense to >> copy quota back to one of the variable instances. To keep things simple, this >> patch undo the copy. >> >> Signed-off-by: Changli Gao <xiaosuo@gmail.com> > > This looks the wrong way to fix this problem. > > Also Changli, could you please _not_ include the title of your patches > inside the Changelog ? This is useless. > Thanks. I remembered some document mentioned a summary line was needed. I'll follow your advice.
On Fri, Jul 23, 2010 at 1:40 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > Reading again your patch, I understand only the Changelog is wrong. > > We want to copy quota back to userspace, as specified when rule was > setup (so that iptables-save works) > > The real thing you are doing is that we dont change the initial quota > during packet processing, only the private quota, shared by all cpus. > > Before the patch , iptables -nvL could report an old and not accurate > quota value. > > After the patch, iptables -nvL reports the initial quota value, not the > actual value. > Yes. This module was expected to report the current quota value, when iptables -nvL or iptables-save was executed. It seems my patch changes its ABI. However, report the initial value is also meaningful, and it keeps the same behavior as the other modules do.
On Friday 2010-07-23 06:54, Changli Gao wrote: >This patch should be applied after my another patch: >http://patchwork.ozlabs.org/patch/59729/ > >xt_quota: don't copy quota back to userspace > >In nowadays, table entries are per-cpu variables, so it don't make any >sense to copy quota back to one of the variable instances. To keep >things simple, this patch undo the copy. I object. This line is on purpose, to give at least a chance of reporting back a more-or-less believable value. Without copying the value back, users have moaned about the counter not decreasing _at all_. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le vendredi 23 juillet 2010 à 08:20 +0200, Jan Engelhardt a écrit : > On Friday 2010-07-23 06:54, Changli Gao wrote: > > >This patch should be applied after my another patch: > >http://patchwork.ozlabs.org/patch/59729/ > > > >xt_quota: don't copy quota back to userspace > > > >In nowadays, table entries are per-cpu variables, so it don't make any > >sense to copy quota back to one of the variable instances. To keep > >things simple, this patch undo the copy. > > I object. This line is on purpose, to give at least a chance of > reporting back a more-or-less believable value. Without copying > the value back, users have moaned about the counter not decreasing > _at all_. Maybe, but current situation is buggy. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23.07.2010 08:28, Eric Dumazet wrote: > Le vendredi 23 juillet 2010 à 08:20 +0200, Jan Engelhardt a écrit : >> On Friday 2010-07-23 06:54, Changli Gao wrote: >> >>> This patch should be applied after my another patch: >>> http://patchwork.ozlabs.org/patch/59729/ >>> >>> xt_quota: don't copy quota back to userspace >>> >>> In nowadays, table entries are per-cpu variables, so it don't make any >>> sense to copy quota back to one of the variable instances. To keep >>> things simple, this patch undo the copy. >> >> I object. This line is on purpose, to give at least a chance of >> reporting back a more-or-less believable value. Without copying >> the value back, users have moaned about the counter not decreasing >> _at all_. > > Maybe, but current situation is buggy. Indeed, besides not being able to properly "iptables-save" a rule, its not possible to delete a specific quota rule since they can't be distinguished based on the specified quota value: # iptables -A INPUT -m quota --quota 1000 # iptables -A INPUT -m quota --quota 2000 # iptables -D INPUT -m quota --quota 2000 # iptables -vxnL INPUT Chain INPUT (policy ACCEPT 2 packets, 96 bytes) pkts bytes target prot opt in out source destination 6 356 all -- * * 0.0.0.0/0 0.0.0.0/0 quota: 1644 bytes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23.07.2010 07:40, Eric Dumazet wrote: > Le vendredi 23 juillet 2010 à 07:27 +0200, Eric Dumazet a écrit : >> Le vendredi 23 juillet 2010 à 12:54 +0800, Changli Gao a écrit : >>> This patch should be applied after my another patch: >>> http://patchwork.ozlabs.org/patch/59729/ >>> >>> xt_quota: don't copy quota back to userspace >>> >>> In nowadays, table entries are per-cpu variables, so it don't make any sense to >>> copy quota back to one of the variable instances. To keep things simple, this >>> patch undo the copy. >>> >>> Signed-off-by: Changli Gao <xiaosuo@gmail.com> >> >> This looks the wrong way to fix this problem. >> >> Also Changli, could you please _not_ include the title of your patches >> inside the Changelog ? This is useless. >> > > Reading again your patch, I understand only the Changelog is wrong. > > We want to copy quota back to userspace, as specified when rule was > setup (so that iptables-save works) > > The real thing you are doing is that we dont change the initial quota > during packet processing, only the private quota, shared by all cpus. > > Before the patch , iptables -nvL could report an old and not accurate > quota value. > > After the patch, iptables -nvL reports the initial quota value, not the > actual value. I've fixed up the changelog and applied the patch, thanks. Changli, please also update the userspace extension to not ignore the quota value on deletion. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netfilter/xt_quota.h b/include/linux/netfilter/xt_quota.h index 8dc89df..b0d28c6 100644 --- a/include/linux/netfilter/xt_quota.h +++ b/include/linux/netfilter/xt_quota.h @@ -11,9 +11,9 @@ struct xt_quota_priv; struct xt_quota_info { u_int32_t flags; u_int32_t pad; + aligned_u64 quota; /* Used internally by the kernel */ - aligned_u64 quota; struct xt_quota_priv *master; }; diff --git a/net/netfilter/xt_quota.c b/net/netfilter/xt_quota.c index 304b1fd..70eb2b4 100644 --- a/net/netfilter/xt_quota.c +++ b/net/netfilter/xt_quota.c @@ -36,8 +36,6 @@ quota_mt(const struct sk_buff *skb, struct xt_action_param *par) /* we do not allow even small packets from now on */ priv->quota = 0; } - /* Copy quota back to matchinfo so that iptables can display it */ - q->quota = priv->quota; spin_unlock_bh(&priv->lock); return ret;
This patch should be applied after my another patch: http://patchwork.ozlabs.org/patch/59729/ xt_quota: don't copy quota back to userspace In nowadays, table entries are per-cpu variables, so it don't make any sense to copy quota back to one of the variable instances. To keep things simple, this patch undo the copy. Signed-off-by: Changli Gao <xiaosuo@gmail.com> ---- include/linux/netfilter/xt_quota.h | 2 +- net/netfilter/xt_quota.c | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html