Quota on SMP AGAIN

Quota on SMP AGAIN

[gpf]

hi

iptables, iptables-save not working properly

[root@ns bind] iptables -L quot |grep devel
RETURN     0    --  anywhere             developer.simm.ru   quota:
226620450 bytes
[root@ns bind] iptables -L quot |grep devel
RETURN     0    --  anywhere             developer.simm.ru   quota:
226620036 bytes
[root@ns bind] iptables -L quot |grep devel
RETURN     0    --  anywhere             developer.simm.ru   quota:
227219801 bytes
[root@ns bind] iptables -L quot |grep devel
RETURN     0    --  anywhere             developer.simm.ru   quota:
227219801 bytes
[root@ns bind] iptables -L quot |grep devel
RETURN     0    --  anywhere             developer.simm.ru   quota:
226619312 bytes

[root@ns bind] uname -a
Linux ns 2.6.18-5-686 #1 SMP Wed Oct 3 00:12:50 UTC 2007 i686 GNU/Linux
[root@ns bind] cat /etc/debian_version
4.0
[root@ns bind] dpkg -l |grep iptables
ii  iptables                   1.3.6.0debian1-5                        
administration tools for packet filtering an

Hardware: HP Proliant DL360

Re: Quota on SMP AGAIN

[Jan Engelhardt]

On Dec 27 2007 18:14, gpf wrote:
>
>iptables, iptables-save not working properly
>
>[root@ns bind] iptables -L quot |grep devel
>RETURN     0    --  anywhere             developer.simm.ru   quota:
>226620450 bytes
>[root@ns bind] iptables -L quot |grep devel
>RETURN     0    --  anywhere             developer.simm.ru   quota:
>226620036 bytes
>[root@ns bind] iptables -L quot |grep devel
>RETURN     0    --  anywhere             developer.simm.ru   quota:
>227219801 bytes
>[root@ns bind] iptables -L quot |grep devel
>RETURN     0    --  anywhere             developer.simm.ru   quota:
>227219801 bytes
>[root@ns bind] iptables -L quot |grep devel
>RETURN     0    --  anywhere             developer.simm.ru   quota:
>226619312 bytes
>
Well at least I can reproduce it fairly easily :-)

Linux ccgmbh 2.6.23.10-ccj62-regular #1 SMP 2007/10/26 14:17:15 UTC x86_64
x86_64 x86_64 GNU/Linux

Re: Quota on SMP AGAIN

[gpf]

Jan Engelhardt ?????:
> On Dec 27 2007 18:14, gpf wrote:
>   
>> iptables, iptables-save not working properly
>>
>> [root@ns bind] iptables -L quot |grep devel
>> RETURN     0    --  anywhere             developer.simm.ru   quota:
>> 226620450 bytes
>> [root@ns bind] iptables -L quot |grep devel
>> RETURN     0    --  anywhere             developer.simm.ru   quota:
>> 226620036 bytes
>> [root@ns bind] iptables -L quot |grep devel
>> RETURN     0    --  anywhere             developer.simm.ru   quota:
>> 227219801 bytes
>> [root@ns bind] iptables -L quot |grep devel
>> RETURN     0    --  anywhere             developer.simm.ru   quota:
>> 227219801 bytes
>> [root@ns bind] iptables -L quot |grep devel
>> RETURN     0    --  anywhere             developer.simm.ru   quota:
>> 226619312 bytes
>>
>>     
> Well at least I can reproduce it fairly easily :-)
>
> Linux ccgmbh 2.6.23.10-ccj62-regular #1 SMP 2007/10/26 14:17:15 UTC x86_64
> x86_64 x86_64 GNU/Linux
>   
kernel upgrade:
[root@ns ~] uname -a
Linux ns 2.6.22-3-686 #1 SMP Tue Dec 4 02:25:59 UTC 2007 i686 GNU/Linux

[root@ns ~] iptables -L quot |grep Yanus
RETURN 0 -- anywhere Yanus.simm.ru quota: 138260287 bytes
[root@ns ~] iptables -L quot |grep Yanus
RETURN 0 -- anywhere Yanus.simm.ru quota: 138258218 bytes
[root@ns ~] iptables -L quot |grep Yanus
RETURN 0 -- anywhere Yanus.simm.ru quota: 138334555 bytes

:((((((

Re: Quota on SMP AGAIN

[Patrick McHardy]

gpf wrote:
> hi
>
> iptables, iptables-save not working properly
>
> [root@ns bind] iptables -L quot |grep devel
> RETURN     0    --  anywhere             developer.simm.ru   quota:
> 226620450 bytes
> [root@ns bind] dpkg -l |grep iptables
> ii  iptables                   1.3.6.0debian1-5
>   


The quota extension in old iptables releases is incompatible with the
version merged upstream.

Re: Quota on SMP AGAIN

[Jan Engelhardt]

On Dec 28 2007 16:26, Patrick McHardy wrote:
> gpf wrote:
>> hi
>>
>> iptables, iptables-save not working properly
>>
>> [root@ns bind] iptables -L quot |grep devel
>> RETURN     0    --  anywhere             developer.simm.ru   quota:
>> 226620450 bytes
>> [root@ns bind] dpkg -l |grep iptables
>> ii  iptables                   1.3.6.0debian1-5
>>   
>
> The quota extension in old iptables releases is incompatible with the
> version merged upstream.

I am seeing this too on 2.6.23.10 with iptables-1.4.0svn7097.
No incompatibilities obvious to me, both kernel and iptables use
the same xt_quota.h.

Re: Quota on SMP AGAIN

[Patrick McHardy]

Jan Engelhardt wrote:
> On Dec 28 2007 16:26, Patrick McHardy wrote:
>   
>> gpf wrote:
>>     
>>> hi
>>>
>>> iptables, iptables-save not working properly
>>>
>>> [root@ns bind] iptables -L quot |grep devel
>>> RETURN     0    --  anywhere             developer.simm.ru   quota:
>>> 226620450 bytes
>>> [root@ns bind] dpkg -l |grep iptables
>>> ii  iptables                   1.3.6.0debian1-5
>>>   
>>>       
>> The quota extension in old iptables releases is incompatible with the
>> version merged upstream.
>>     
>
> I am seeing this too on 2.6.23.10 with iptables-1.4.0svn7097.
> No incompatibilities obvious to me, both kernel and iptables use
> the same xt_quota.h.

Gpf's report didn't include an actual description of the problem, so
I can only assume its an incorrect value of the quota. What exactly
are the effects you're seeing?

Re: Quota on SMP AGAIN

[Jan Engelhardt]

On Dec 28 2007 17:13, Patrick McHardy wrote:
>> > >
>> > > [root@ns bind] iptables -L quot |grep devel
>> > > RETURN     0    --  anywhere             developer.simm.ru   quota:
>> > > 226620450 bytes
>> > > [root@ns bind] dpkg -l |grep iptables
>> > > ii  iptables                   1.3.6.0debian1-5
>> > >   
>> > >       
>> > The quota extension in old iptables releases is incompatible with the
>> > version merged upstream.
>>
>> I am seeing this too on 2.6.23.10 with iptables-1.4.0svn7097.
>> No incompatibilities obvious to me, both kernel and iptables use
>> the same xt_quota.h.
>
> Gpf's report didn't include an actual description of the problem, so
> I can only assume its an incorrect value of the quota. What exactly
> are the effects you're seeing?
>

To begin with:

	ssh root@somebox

when logged in:

	iptables -I OUTPUT -m quota --quota 1234567

as you do more actions over the interactive ssh session, the quota
will obviously decrease -- after all, that is what it is supposed
to do. Enter

	iptables -nvL OUTPUT

should reduce the quota by some 50-60 bytes per packet (and typing the 
iptables command costs some bytes), but as you 
repeatedly call iptables, the quota fluctuates around 1234567.

What one can see is that the packet counter (1st column) increases while 
the quota has problems.

17:38 ccgmbh:~ # iptables -nvL | grep quota
  155 35709            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1234567 bytes
17:38 ccgmbh:~ # iptables -nvL | grep quota
  163 36609            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1234567 bytes
17:38 ccgmbh:~ # iptables -nvL | grep quota
  171 37509            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1197058 bytes
17:38 ccgmbh:~ # iptables -nvL | grep quota
  179 38409            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1234567 bytes

What is interesting to note is that if we do away with all "wrong" 
results, then it "works":

17:39 ccgmbh:~ # while :; do iptables -nvL | grep quota; done | grep -v 1234567
...
  830  118K            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1116680 bytes
  832  118K            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1116399 bytes
  834  118K            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1116150 bytes
  834  118K            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1116150 bytes
  838  119K            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1115684 bytes
...

Want to know what I believe? That r->master = r in the source code is 
redundant and/or does not solve the problem its comment says.

/* For SMP, we only want to use one set of counters. */

And my bigtime question would be: where is the other counter actually? 
struct xt_quota_info only has one counter! Does netfilter secretly 
allocate matchinfos per-cpu?

Re: Quota on SMP AGAIN

[Patrick McHardy]

Jan Engelhardt wrote:
> On Dec 28 2007 17:13, Patrick McHardy wrote:
>   
>> Gpf's report didn't include an actual description of the problem, so
>> I can only assume its an incorrect value of the quota. What exactly
>> are the effects you're seeing?
>>
>>     
>
> To begin with:
>
> 	ssh root@somebox
>
> when logged in:
>
> 	iptables -I OUTPUT -m quota --quota 1234567
>
> as you do more actions over the interactive ssh session, the quota
> will obviously decrease -- after all, that is what it is supposed
> to do. Enter
>
> 	iptables -nvL OUTPUT
>
> should reduce the quota by some 50-60 bytes per packet (and typing the 
> iptables command costs some bytes), but as you 
> repeatedly call iptables, the quota fluctuates around 1234567.
>
> What one can see is that the packet counter (1st column) increases while 
> the quota has problems.
>
> 17:38 ccgmbh:~ # iptables -nvL | grep quota
>   155 35709            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1234567 bytes
> 17:38 ccgmbh:~ # iptables -nvL | grep quota
>   163 36609            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1234567 bytes
> 17:38 ccgmbh:~ # iptables -nvL | grep quota
>   171 37509            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1197058 bytes
> 17:38 ccgmbh:~ # iptables -nvL | grep quota
>   179 38409            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1234567 bytes
>
> What is interesting to note is that if we do away with all "wrong" 
> results, then it "works":
>
> 17:39 ccgmbh:~ # while :; do iptables -nvL | grep quota; done | grep -v 1234567
> ...
>   830  118K            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1116680 bytes
>   832  118K            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1116399 bytes
>   834  118K            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1116150 bytes
>   834  118K            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1116150 bytes
>   838  119K            all  --  *      *       0.0.0.0/0            0.0.0.0/0           quota: 1115684 bytes
> ...
>
> Want to know what I believe? That r->master = r in the source code is 
> redundant and/or does not solve the problem its comment says.
>
> /* For SMP, we only want to use one set of counters. */
>
> And my bigtime question would be: where is the other counter actually? 
> struct xt_quota_info only has one counter! Does netfilter secretly 
> allocate matchinfos per-cpu?
>   

Not secretly, but yes, the entire ruleset exists once per CPU. That
also seems to be the problem, at the time the master idea was thought
of we always dumped entries from CPU 0, today its from the current
CPU, but the only one that actually has correct counters is CPU 0.

The easy fix would be to revert to that behaviour, but maybe someone
can come up with a better idea that doesn't involve walking over the
entire ruleset and resycing things or adding dump callbacks to
matches.

Re: Quota on SMP AGAIN

[Jan Engelhardt]

On Dec 28 2007 17:50, Patrick McHardy wrote:
>>
>> And my bigtime question would be: where is the other counter
>> actually? struct xt_quota_info only has one counter! Does
>> netfilter secretly allocate matchinfos per-cpu?
>
> Not secretly, but yes, the entire ruleset exists once per CPU. That
> also seems to be the problem, at the time the master idea was thought
> of we always dumped entries from CPU 0, today its from the current
> CPU, but the only one that actually has correct counters is CPU 0.
>
> The easy fix would be to revert to that behaviour, but maybe someone
> can come up with a better idea that doesn't involve walking over the
> entire ruleset and resycing things or adding dump callbacks to
> matches.
>
Well, the short term fix would be to turn

	q->master = q

into

	q->master = percpu_get(0, q)

or whatever is appropriate.

In the long run, separating the rule from the matchinfo data would be
beneficial. Or actually - why cannot the ruleset exists once for all
CPUs? The write path is a slow path anyway / it is read-mostly anyway.

Re: Quota on SMP AGAIN

[Patrick McHardy]

Jan Engelhardt wrote:
> On Dec 28 2007 17:50, Patrick McHardy wrote:
>   
>>> And my bigtime question would be: where is the other counter
>>> actually? struct xt_quota_info only has one counter! Does
>>> netfilter secretly allocate matchinfos per-cpu?
>>>       
>> Not secretly, but yes, the entire ruleset exists once per CPU. That
>> also seems to be the problem, at the time the master idea was thought
>> of we always dumped entries from CPU 0, today its from the current
>> CPU, but the only one that actually has correct counters is CPU 0.
>>
>> The easy fix would be to revert to that behaviour, but maybe someone
>> can come up with a better idea that doesn't involve walking over the
>> entire ruleset and resycing things or adding dump callbacks to
>> matches.
>>
>>     
> Well, the short term fix would be to turn
>
> 	q->master = q
>
> into
>
> 	q->master = percpu_get(0, q)
>
> or whatever is appropriate.
>   

That doesn't help, the problem is that we keep only the counters on
CPU0 up to date, but the data copied to userspace during iptables -L
is chosen by the CPU the iptables command is running on.

> In the long run, separating the rule from the matchinfo data would be
> beneficial. Or actually - why cannot the ruleset exists once for all
> CPUs? The write path is a slow path anyway / it is read-mostly anyway.
>   

Mainly because of the counters, we don't want to have all CPUs fighting
for the cachelines.

Re: Quota on SMP AGAIN

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1362 bytes --]



On Sat, 29 Dec 2007, Patrick McHardy wrote:

> Jan Engelhardt wrote:
>> On Dec 28 2007 17:50, Patrick McHardy wrote:
>> 
>>>> And my bigtime question would be: where is the other counter
>>>> actually? struct xt_quota_info only has one counter! Does
>>>> netfilter secretly allocate matchinfos per-cpu?
>>>> 
>>> Not secretly, but yes, the entire ruleset exists once per CPU. That
>>> also seems to be the problem, at the time the master idea was thought
>>> of we always dumped entries from CPU 0, today its from the current
>>> CPU, but the only one that actually has correct counters is CPU 0.
>>> 
>>> The easy fix would be to revert to that behaviour, but maybe someone
>>> can come up with a better idea that doesn't involve walking over the
>>> entire ruleset and resycing things or adding dump callbacks to
>>> matches.
>>>
>>> 
>> Well, the short term fix would be to turn
>>
>> 	q->master = q
>> 
>> into
>>
>> 	q->master = percpu_get(0, q)
>> 
>> or whatever is appropriate.
>> 
>
> That doesn't help, the problem is that we keep only the counters on
> CPU0 up to date, but the data copied to userspace during iptables -L
> is chosen by the CPU the iptables command is running on.

As a short-term workaround one can use taskset to force running iptables 
on CPU#0.


Best regards,

 					Krzysztof Olędzki

Re: Quota on SMP AGAIN

[Jan Engelhardt]

On Dec 29 2007 19:54, Krzysztof Oledzki wrote:
>>
>> That doesn't help, the problem is that we keep only the counters on
>> CPU0 up to date, but the data copied to userspace during iptables -L
>> is chosen by the CPU the iptables command is running on.
>
> As a short-term workaround one can use taskset to force running iptables on
> CPU#0.
>

Actually, on one (but fixed) arbitrary core.

Re: Quota on SMP AGAIN

[Patrick McHardy]

Jan Engelhardt wrote:
> On Dec 29 2007 19:54, Krzysztof Oledzki wrote:
>   
>>> That doesn't help, the problem is that we keep only the counters on
>>> CPU0 up to date, but the data copied to userspace during iptables -L
>>> is chosen by the CPU the iptables command is running on.
>>>       
>> As a short-term workaround one can use taskset to force running iptables on
>> CPU#0.
>>
>>     
>
> Actually, on one (but fixed) arbitrary core.

Indeed, what a mess. I was just about to commit a patch to
always use first_cpu(cpu_possible_map) for copy_entries_to_user,
but thats no enough, we also need to force ruleset replacement
to choose the same CPU. This effectively eliminates all performance
improvements of the NUMA optimizations during ruleset updates.

Its ugly (not much more than other parts of iptables though),
but we should be able to keep half the improvement by using
raw_smp_processor_id() for ruleset replacements and storing
that number for the next copy_entries_to_user operations.

Alternatively we could flag the matches and targets requiring
this and fix them up during the second pass (counter fixup).
Most of them are rather rarely used I guess (limit, hashlimit,
quota and statistic).

Eric, do you have any better suggestions how to fix this?

Re: Quota on SMP AGAIN

[Jan Engelhardt]

On Dec 30 2007 18:36, Patrick McHardy wrote:
> Jan Engelhardt wrote:
>> On Dec 29 2007 19:54, Krzysztof Oledzki wrote:
>>   
>> > > That doesn't help, the problem is that we keep only the counters on
>> > > CPU0 up to date, but the data copied to userspace during iptables -L
>> > > is chosen by the CPU the iptables command is running on.
>> > >       
>> > As a short-term workaround one can use taskset to force running iptables on
>> > CPU#0.
>>
>> Actually, on one (but fixed) arbitrary core.
>
> Its ugly (not much more than other parts of iptables though),
> but we should be able to keep half the improvement by using
> raw_smp_processor_id() for ruleset replacements and storing
> that number for the next copy_entries_to_user operations.
>
> Alternatively we could flag the matches and targets requiring
> this and fix them up during the second pass (counter fixup).
> Most of them are rather rarely used I guess (limit, hashlimit,
> quota and statistic).

Hmph, how are matches supposed to work with it at all?
Assume this:

static int ...check...(...)
{
	struct my_match_info *info = matchinfo;

	info->foobar = kmalloc(...);
}

->checkentry() is only called once for each rule, but if each
rule has its own data space (e.g. info->foobar), then that sucks.

Re: Quota on SMP AGAIN

[Patrick McHardy]

Jan Engelhardt wrote:
> Hmph, how are matches supposed to work with it at all?
> Assume this:
>
> static int ...check...(...)
> {
> 	struct my_match_info *info = matchinfo;
>
> 	info->foobar = kmalloc(...);
> }
>
> ->checkentry() is only called once for each rule, but if each
> rule has its own data space (e.g. info->foobar), then that sucks.
>   

->checkentry is called before the copying, so all CPUs get the same
foobar pointer.

Re: Quota on SMP AGAIN

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 624 bytes --]



On Fri, 28 Dec 2007, Patrick McHardy wrote:
<CUT>
>> And my bigtime question would be: where is the other counter actually? 
>> struct xt_quota_info only has one counter! Does netfilter secretly allocate 
>> matchinfos per-cpu?
>> 
>
> Not secretly, but yes, the entire ruleset exists once per CPU. That
> also seems to be the problem, at the time the master idea was thought
> of we always dumped entries from CPU 0, today its from the current
> CPU, but the only one that actually has correct counters is CPU 0.

What happens when CPU#0 is disabled (CPU hotplug)?

Best regards,

 				Krzysztof Olędzki

Re: Quota on SMP AGAIN

[Patrick McHardy]

Krzysztof Oledzki wrote:
>
>
> On Fri, 28 Dec 2007, Patrick McHardy wrote:
> <CUT>
>>> And my bigtime question would be: where is the other counter 
>>> actually? struct xt_quota_info only has one counter! Does netfilter 
>>> secretly allocate matchinfos per-cpu?
>>>
>>
>> Not secretly, but yes, the entire ruleset exists once per CPU. That
>> also seems to be the problem, at the time the master idea was thought
>> of we always dumped entries from CPU 0, today its from the current
>> CPU, but the only one that actually has correct counters is CPU 0.
>
> What happens when CPU#0 is disabled (CPU hotplug)?

Nothing, its simply unused except for counter synchronization and
the ->master thing this thread is about.

Re: Quota on SMP AGAIN

[Jan Engelhardt]

On Dec 31 2007 01:19, Patrick McHardy wrote:
>> On Fri, 28 Dec 2007, Patrick McHardy wrote:
>> <CUT>
>> > > And my bigtime question would be: where is the other counter actually?
>> > > struct xt_quota_info only has one counter! Does netfilter secretly
>> > > allocate matchinfos per-cpu?
>> > >
>> >
>> > Not secretly, but yes, the entire ruleset exists once per CPU. That
>> > also seems to be the problem, at the time the master idea was thought
>> > of we always dumped entries from CPU 0, today its from the current
>> > CPU, but the only one that actually has correct counters is CPU 0.
>>
>> What happens when CPU#0 is disabled (CPU hotplug)?
>
> Nothing, its simply unused except for counter synchronization and
> the ->master thing this thread is about.
>

01:57 ccgmbh:~ # taskset 1 iptables -I OUTPUT -m quota --quota 123456

Now suppose CPU#0 was deactivated (taskset only to show what's meant).
Then iptables would always show the wrong quota.

Re: Quota on SMP AGAIN

[Patrick McHardy]

Jan Engelhardt wrote:
> On Dec 31 2007 01:19, Patrick McHardy wrote:
>   
>>>
>>> What happens when CPU#0 is disabled (CPU hotplug)?
>>>       
>> Nothing, its simply unused except for counter synchronization and
>> the ->master thing this thread is about.
>>
>>     
>
> 01:57 ccgmbh:~ # taskset 1 iptables -I OUTPUT -m quota --quota 123456
>
> Now suppose CPU#0 was deactivated (taskset only to show what's meant).
> Then iptables would always show the wrong quota.
>   

Sure, because taskset is a workaround and not a fix for the problem.
Its still the exact same bug.