[patch] Fix ipt_ACCOUNT for large networks - 2nd try

[patch] Fix ipt_ACCOUNT for large networks - 2nd try

[Thomas Jarosch]

Hi Patrick,

attached is a patch to fix the ACCOUNT target for large networks.
This time for good ;-) Discard the patch I sent yesterday.

It runs stable now on a network with ~1300 active clients. Quoting the user:
"Nevertheless, from performance point of view, ipt_ACCOUNT rocks. I have a
distribution for routers (Route Hat) and last night switched from ipt_account
to ipt_ACCOUNT completely. What took half a minute before now takes 300ms
i.e. 100 times faster, same router, same net. Gr8 work :-)"

Cheers,
Thomas

Signed-off-by: Thomas Jarosch <thomas.jarosch@intra2net.com>

diff -u -r -b ACCOUNT/linux/net/ipv4/netfilter/ipt_ACCOUNT.c ACCOUNT.1.4/linux/net/ipv4/netfilter/ipt_ACCOUNT.c
--- ACCOUNT/linux/net/ipv4/netfilter/ipt_ACCOUNT.c      2004-06-13 22:41:21.000000000 +0200
+++ ACCOUNT.1.4/linux/net/ipv4/netfilter/ipt_ACCOUNT.c  2005-04-05 09:33:52.000000000 +0200
@@ -3,7 +3,7 @@
  *   See http://www.intra2net.com/opensource/ipt_account                   *
  *   for further information                                               *
  *                                                                         *
- *   Copyright (C) 2004 by Intra2net AG                                    *
+ *   Copyright (C) 2004-2005 by Intra2net AG                               *
  *   opensource@intra2net.com                                              *
  *                                                                         *
  *   This program is free software; you can redistribute it and/or modify  *
@@ -694,7 +694,8 @@
 /* Copy 8 bit network data into a prepared buffer.
    We only copy entries != 0 to increase performance.
 */
-static int ipt_acc_handle_copy_data(void *to_user, int *pos,
+static int ipt_acc_handle_copy_data(void *to_user, u_int32_t *to_user_pos,
+                                  u_int32_t *tmpbuf_pos,
                                     struct ipt_acc_mask_24 *data,
                                     u_int32_t net_ip, u_int32_t net_OR_mask)
 {
@@ -712,13 +713,15 @@
             handle_ip.dst_bytes = data->ip[i].dst_bytes;

             /* Temporary buffer full? Flush to userspace */
-            if (*pos+handle_ip_size >= PAGE_SIZE) {
-                *pos = 0;
-                if (copy_to_user(to_user, ipt_acc_tmpbuf, *pos))
+            if (*tmpbuf_pos+handle_ip_size >= PAGE_SIZE) {
+                if (copy_to_user(to_user + *to_user_pos, ipt_acc_tmpbuf,
+                                                           *tmpbuf_pos))
                     return -EFAULT;
+                *to_user_pos = *to_user_pos + *tmpbuf_pos;
+                *tmpbuf_pos = 0;
             }
-            memcpy(ipt_acc_tmpbuf+*pos, &handle_ip, handle_ip_size);
-            *pos += handle_ip_size;
+            memcpy(ipt_acc_tmpbuf+*tmpbuf_pos, &handle_ip, handle_ip_size);
+            *tmpbuf_pos += handle_ip_size;
         }
     }

@@ -731,7 +734,7 @@
 */
 static int ipt_acc_handle_get_data(u_int32_t handle, void *to_user)
 {
-    u_int32_t tmpbuf_pos=0, net_ip;
+    u_int32_t to_user_pos=0, tmpbuf_pos=0, net_ip;
     unsigned char depth;

     if (handle >= ACCOUNT_MAX_HANDLES) {
@@ -752,12 +755,13 @@
     if (depth == 0) {
         struct ipt_acc_mask_24 *network =
             (struct ipt_acc_mask_24*)ipt_acc_handles[handle].data;
-        if (ipt_acc_handle_copy_data(to_user, &tmpbuf_pos, network, net_ip, 0))
+        if (ipt_acc_handle_copy_data(to_user, &to_user_pos, &tmpbuf_pos,
+                                     network, net_ip, 0))
             return -1;

         /* Flush remaining data to userspace */
         if (tmpbuf_pos)
-            if (copy_to_user(to_user, ipt_acc_tmpbuf, tmpbuf_pos))
+            if (copy_to_user(to_user+to_user_pos, ipt_acc_tmpbuf, tmpbuf_pos))
                 return -1;

         return 0;
@@ -772,15 +776,15 @@
             if (network_16->mask_24[b]) {
                 struct ipt_acc_mask_24 *network =
                     (struct ipt_acc_mask_24*)network_16->mask_24[b];
-                if (ipt_acc_handle_copy_data(to_user, &tmpbuf_pos, network,
-                                             net_ip, (b << 16)))
+                if (ipt_acc_handle_copy_data(to_user, &to_user_pos,
+                                      &tmpbuf_pos, network, net_ip, (b << 16)))
                     return -1;
             }
         }

         /* Flush remaining data to userspace */
         if (tmpbuf_pos)
-            if (copy_to_user(to_user, ipt_acc_tmpbuf, tmpbuf_pos))
+            if (copy_to_user(to_user+to_user_pos, ipt_acc_tmpbuf, tmpbuf_pos))
                 return -1;

         return 0;
@@ -799,7 +803,8 @@
                     if (network_16->mask_24[b]) {
                         struct ipt_acc_mask_24 *network =
                             (struct ipt_acc_mask_24*)network_16->mask_24[b];
-                        if (ipt_acc_handle_copy_data(to_user, &tmpbuf_pos,
+                        if (ipt_acc_handle_copy_data(to_user,
+                                       &to_user_pos, &tmpbuf_pos,
                                         network, net_ip, (a << 8) | (b << 16)))
                             return -1;
                     }
@@ -809,7 +814,7 @@

         /* Flush remaining data to userspace */
         if (tmpbuf_pos)
-            if (copy_to_user(to_user, ipt_acc_tmpbuf, tmpbuf_pos))
+            if (copy_to_user(to_user+to_user_pos, ipt_acc_tmpbuf, tmpbuf_pos))
                 return -1;

         return 0;
diff -u -r -b ACCOUNT/linux/include/linux/netfilter_ipv4/ipt_ACCOUNT.h ACCOUNT.1.4/linux/include/linux/netfilter_ipv4/ipt_ACCOUNT.h
--- ACCOUNT/linux/include/linux/netfilter_ipv4/ipt_ACCOUNT.h    2004-06-13 22:41:21.000000000 +0200
+++ ACCOUNT.1.4/linux/include/linux/netfilter_ipv4/ipt_ACCOUNT.h        2005-04-05 09:24:20.995818974 +0200
@@ -1,5 +1,5 @@
 /***************************************************************************
- *   Copyright (C) 2004 by Intra2net AG                                    *
+ *   Copyright (C) 2004-2005 by Intra2net AG                               *
  *   opensource@intra2net.com                                              *
  *                                                                         *
  *   This program is free software; you can redistribute it and/or modify  *

Re: [patch] Fix ipt_ACCOUNT for large networks - 2nd try

[Carl-Daniel Hailfinger]

Hi Thomas,

Thomas Jarosch schrieb:
> 
> attached is a patch to fix the ACCOUNT target for large networks.
> This time for good ;-) Discard the patch I sent yesterday.
> 
> It runs stable now on a network with ~1300 active clients. Quoting the user:
> "Nevertheless, from performance point of view, ipt_ACCOUNT rocks. I have a
> distribution for routers (Route Hat) and last night switched from ipt_account
> to ipt_ACCOUNT completely. What took half a minute before now takes 300ms
> i.e. 100 times faster, same router, same net. Gr8 work :-)"

When reading the source, it seems to me that ACCOUNT has some room
for improvement:

- It is handling some sparsely populated networks inefficiently.
Consider e.g. a 192.168/16 network with 512 clients spread over
the whole range like 192.168.0.10,192.168.0.25,192.168.1.12 etc.
This needs as much memory as a fully populated network of the same
size.
- It can only handle /8,/16 and /24 networks.
- It can't account based on MAC or MAC/IP.
- It is impossible to select what is accounted for (packets/bytes,
in/out) and what should be added to the counters (layer 2 frame
length, layer 3 packet length...).

Oh, and I'd like to run the code on a 64bit machine with a 2.6 kernel.

Do you accept patches for the above items or are they already done/
being worked on by somebody else?


Regards,
Carl-Daniel
-- 
http://www.hailfinger.org/

Re: [patch] Fix ipt_ACCOUNT for large networks - 2nd try

[Thomas Jarosch]

Hi Carl-Daniel,

> When reading the source, it seems to me that ACCOUNT has some room
> for improvement:
>
> - It is handling some sparsely populated networks inefficiently.
> Consider e.g. a 192.168/16 network with 512 clients spread over
> the whole range like 192.168.0.10,192.168.0.25,192.168.1.12 etc.
> This needs as much memory as a fully populated network of the same
> size.
> - It can only handle /8,/16 and /24 networks.
> - It can't account based on MAC or MAC/IP.
> - It is impossible to select what is accounted for (packets/bytes,
> in/out) and what should be added to the counters (layer 2 frame
> length, layer 3 packet length...).
>
> Oh, and I'd like to run the code on a 64bit machine with a 2.6 kernel.
>
> Do you accept patches for the above items or are they already done/
> being worked on by somebody else?

I'll gladly accept patches to it. Guess nobody is currently doing any 
development (except for bugfixes) as it's working perfect for us.

IIRC the future of the accounting code is conntrack_acct.
Packets have to pass conntrack anyway, so doing the accounting
there is a good idea. Maybe it's better to test/enhance conntrack_acct?
(Not that I don't like my own ipt_ACCOUNT code ;-))

The question is, can the accounting information in conntrack_acct
be queried after the connection is closed/gone? From looking at the source,
it seems the accounting information has the same lifetime
as the conntrack of a connection. Harald, is that true?

The nice part about ACCOUNT is that you can really specify what gets
accounted and what not. This might be hard to achieve with conntrack_acct.
A userspace tool might have to split the data afterwards.

Cheers,
Thomas

Re: [patch] Fix ipt_ACCOUNT for large networks - 2nd try

[Carl-Daniel Hailfinger]

Thomas Jarosch schrieb:
> Hi Patrick,
> 
> attached is a patch to fix the ACCOUNT target for large networks.
> This time for good ;-) Discard the patch I sent yesterday.
> 
> It runs stable now on a network with ~1300 active clients. Quoting the user:
> "Nevertheless, from performance point of view, ipt_ACCOUNT rocks. I have a
> distribution for routers (Route Hat) and last night switched from ipt_account
> to ipt_ACCOUNT completely. What took half a minute before now takes 300ms
> i.e. 100 times faster, same router, same net. Gr8 work :-)"
> 
> Cheers,
> Thomas
> 
> Signed-off-by: Thomas Jarosch <thomas.jarosch@intra2net.com>
> 
> diff -u -r -b ACCOUNT/linux/net/ipv4/netfilter/ipt_ACCOUNT.c ACCOUNT.1.4/linux/net/ipv4/netfilter/ipt_ACCOUNT.c
> --- ACCOUNT/linux/net/ipv4/netfilter/ipt_ACCOUNT.c      2004-06-13 22:41:21.000000000 +0200
> +++ ACCOUNT.1.4/linux/net/ipv4/netfilter/ipt_ACCOUNT.c  2005-04-05 09:33:52.000000000 +0200
> @@ -694,7 +694,8 @@
>  /* Copy 8 bit network data into a prepared buffer.
>     We only copy entries != 0 to increase performance.
>  */
> -static int ipt_acc_handle_copy_data(void *to_user, int *pos,
> +static int ipt_acc_handle_copy_data(void *to_user, u_int32_t *to_user_pos,
> +                                  u_int32_t *tmpbuf_pos,
>                                      struct ipt_acc_mask_24 *data,
>                                      u_int32_t net_ip, u_int32_t net_OR_mask)
>  {

You seem to like u_int32_t as a type. That causes interesting behaviour
on 64bit machines. Is there any design objective dictating that?
I have a patch available changing most occurences of u_int32_t to
something more generic (of course not for IPs, netmasks and such) which
may make sense if you ever want to use your module on 64bit machines.

Regards,
Carl-Daniel
-- 
http://www.hailfinger.org/

Re: [patch] Fix ipt_ACCOUNT for large networks - 2nd try

[Thomas Jarosch]

> > +static int ipt_acc_handle_copy_data(void *to_user, u_int32_t
> > *to_user_pos, +                                  u_int32_t *tmpbuf_pos,
> >                                      struct ipt_acc_mask_24 *data,
> >                                      u_int32_t net_ip, u_int32_t
> > net_OR_mask) {
>
> You seem to like u_int32_t as a type. That causes interesting behaviour
> on 64bit machines. Is there any design objective dictating that?
> I have a patch available changing most occurences of u_int32_t to
> something more generic (of course not for IPs, netmasks and such) which
> may make sense if you ever want to use your module on 64bit machines.

Yep, I think for pointers this change is necessary on 64bit platforms.
Please base your patch on the locking patch I just sent.

Thomas

Re: [patch] Fix ipt_ACCOUNT for large networks - 2nd try

[Patrick McHardy]

Thomas Jarosch wrote:
> attached is a patch to fix the ACCOUNT target for large networks.
> This time for good ;-) Discard the patch I sent yesterday.

Please resend as attachment, your mailer converted tabs to spaces.

patching file linux/net/ipv4/netfilter/ipt_ACCOUNT.c
Hunk #1 FAILED at 3.
Hunk #2 FAILED at 694.
Hunk #3 FAILED at 713.
Hunk #4 FAILED at 734.
Hunk #5 FAILED at 755.
Hunk #6 FAILED at 776.
Hunk #7 FAILED at 803.
Hunk #8 FAILED at 814.
8 out of 8 hunks FAILED -- saving rejects to file 
linux/net/ipv4/netfilter/ipt_ACCOUNT.c.rej

Regards
Patrick

Re: [patch] Fix ipt_ACCOUNT for large networks - 2nd try

[Patrick McHardy]

Carl-Daniel Hailfinger wrote:
>>diff -u -r -b ACCOUNT/linux/net/ipv4/netfilter/ipt_ACCOUNT.c ACCOUNT.1.4/linux/net/ipv4/netfilter/ipt_ACCOUNT.c
>>--- ACCOUNT/linux/net/ipv4/netfilter/ipt_ACCOUNT.c      2004-06-13 22:41:21.000000000 +0200
>>+++ ACCOUNT.1.4/linux/net/ipv4/netfilter/ipt_ACCOUNT.c  2005-04-05 09:33:52.000000000 +0200
>>@@ -694,7 +694,8 @@
>> /* Copy 8 bit network data into a prepared buffer.
>>    We only copy entries != 0 to increase performance.
>> */
>>-static int ipt_acc_handle_copy_data(void *to_user, int *pos,
>>+static int ipt_acc_handle_copy_data(void *to_user, u_int32_t *to_user_pos,
>>+                                  u_int32_t *tmpbuf_pos,
>>                                     struct ipt_acc_mask_24 *data,
>>                                     u_int32_t net_ip, u_int32_t net_OR_mask)
>> {
> 
> 
> You seem to like u_int32_t as a type. That causes interesting behaviour
> on 64bit machines. Is there any design objective dictating that?
> I have a patch available changing most occurences of u_int32_t to
> something more generic (of course not for IPs, netmasks and such) which
> may make sense if you ever want to use your module on 64bit machines.

Seems to be fine, AFAICT the u_int32_ts are only used as offsets.

Regards
Patrick

Re: [patch] Fix ipt_ACCOUNT for large networks - 2nd try

[Thomas Jarosch]

Hi Patrick,

> Please resend as attachment, your mailer converted tabs to spaces.
>
> patching file linux/net/ipv4/netfilter/ipt_ACCOUNT.c
> Hunk #1 FAILED at 3.
> Hunk #2 FAILED at 694.
> Hunk #3 FAILED at 713.
> Hunk #4 FAILED at 734.
> Hunk #5 FAILED at 755.
> Hunk #6 FAILED at 776.
> Hunk #7 FAILED at 803.
> Hunk #8 FAILED at 814.
> 8 out of 8 hunks FAILED -- saving rejects to file
> linux/net/ipv4/netfilter/ipt_ACCOUNT.c.rej

It's already in ;-)

Thomas