From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle. Date: Mon, 09 Nov 2020 14:04:45 +0000 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3410627596928742955==" List-Id: --===============3410627596928742955== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > On 9 Nov 2020, at 12:13, Bernhard Bitsch wrote: >=20 > Hello, >=20 >> Gesendet: Montag, 09. November 2020 um 12:35 Uhr >> Von: "Michael Tremer" >> An: "Bernhard Bitsch" >> Cc: development(a)lists.ipfire.org >> Betreff: Re: [PATCH 1/5] location-functions.pl: Use a single script-wide d= b_handle. >>=20 >> Hello, >>=20 >> Thank you Stefan for submitting this patchset. >>=20 >>> On 8 Nov 2020, at 19:36, Bernhard Bitsch wrote: >>>=20 >>> This means, we stay with the unbalanced memory allocation in (Perl) liblo= c. Which leaves a memory leak. >>=20 >> Bernhard, could you please elaborate on how this memory leak is still exis= ting? >>=20 >> I also do not understand what you mean by unbalanced. >>=20 >> As far as I understand the code right now, the database is being opened on= ce and the handle is being stored internally. All functions that are being ca= lled will no longer have to hold their own database handle. Therefore the max= imum amount of handles open is one. >>=20 >=20 > The init() function allocates the handle, which is not really destroyed. Th= e END block is just a safety process. The allocated memory for the handle sho= uld be released in the moment of references=3D=3D0. The handle is being initialised when location-functions.pl is being loaded an= d it is freed when the Perl interpreter ends. It remains referenced all the time. > Yes, the new code ( inspired by my work-around ) minimizes the number of ha= ndles to one. But this handle is persistent also ( this doesn't hurt because = it is used persistent for the time of module use ). It is only being initialised when location-functions.pl is being loaded. That= only happens in code that uses those functions. I do not see the problem with this. Constantly closing and re-opening the dat= abase is not an option since the connection tracking list can have tens of th= ousands of lookups and this will make the page - simply - slow. I cannot see an option that is more resource-friendly than this. >=20 >>>=20 >>>> Gesendet: Samstag, 07. November 2020 um 19:47 Uhr >>>> Von: "Stefan Schantl" >>>> An: development(a)lists.ipfire.org >>>> Betreff: [PATCH 1/5] location-functions.pl: Use a single script-wide db_= handle. >>>>=20 >>>> Create and use a single script-wide database handle for libloc to >>>> prevent from creating multiple ones. >>>>=20 >>>> This helps saving memory, especially on small systems. >>>>=20 >>>> Reference #12515. >>>>=20 >>>=20 >>> The error can be produced easily with small memory, but it is present in = all systems. >>> Therefore I've posted this solution as work-around only! >>=20 >> Did you test this patchset or did you come to your conclusion by only read= ing the code? >>=20 >> Best, >> -Michael >>=20 >=20 > The sentence about "small systems" refers to the original comment. It is no= t a real memory saving, the patch cures a 'memory wasting'. It is clear, that= a massively use of malloc without free crashes a small system faster than a = system with big memory resources. Opening the database requires pretty much nothing (maybe 4K) of RSS memory an= d about 50MB of VIRT memory. This due to mmap() which helps us reading the da= tabase a lot fast. I think this is very very reasonable. The more data is bei= ng read from the database the more memory it will use. If you wish to change that (and that comes with a performance penalty) you co= uld submit a patch that removes mmap() if the user wishes to. But I do not see why this is a problem at all. The whole system won=E2=80=99t= run well on systems with 128 or 256MB of memory. The Linux kernel won=E2=80= =99t even boot properly on those any more. Memory is cheap. Our time isn=E2= =80=99t. We have found the most resource-saving way to deal with this now and anything= else will simply use considerable amount of time with no benefit to 99% of o= ur users. Best, -Michael > The patchset is mainly my work-around, thus it is in test since I found the= error. >=20 > Best, > -Bernhard >=20 >>>=20 >>> - Bernhard >>>=20 >>>=20 >>>> Signed-off-by: Stefan Schantl >>>> --- >>>> config/cfgroot/location-functions.pl | 11 ++++------- >>>> 1 file changed, 4 insertions(+), 7 deletions(-) >>>>=20 >>>> diff --git a/config/cfgroot/location-functions.pl b/config/cfgroot/locat= ion-functions.pl >>>> index 2cfe7f908..9b1d0bfb5 100644 >>>> --- a/config/cfgroot/location-functions.pl >>>> +++ b/config/cfgroot/location-functions.pl >>>> @@ -55,6 +55,9 @@ our $keyfile =3D "$location_dir/signing-key.pem"; >>>> # Directory which contains the exported databases. >>>> our $xt_geoip_db_directory =3D "/usr/share/xt_geoip/"; >>>>=20 >>>> +# Create libloc database handle. >>>> +my $db_handle =3D &init(); >>>> + >>>> # >>>> ## Tiny function to init the location database. >>>> # >>>> @@ -86,7 +89,7 @@ sub verify ($) { >>>> ## Function to the the country code of a given address. >>>> # >>>> sub lookup_country_code($$) { >>>> - my ($db_handle, $address) =3D @_; >>>> + my ($address) =3D @_; >>>>=20 >>>> # Lookup the given address. >>>> my $country_code =3D &Location::lookup_country_code($db_handle, $addres= s); >>>> @@ -174,9 +177,6 @@ sub get_full_country_name($) { >>>>=20 >>>> # Function to get all available locations. >>>> sub get_locations() { >>>> - # Create libloc database handle. >>>> - my $db_handle =3D &init(); >>>> - >>>> # Get locations which are stored in the location database. >>>> my @database_locations =3D &Location::database_countries($db_handle); >>>>=20 >>>> @@ -197,9 +197,6 @@ sub address_has_flags($) { >>>> # Array to store the flags of the address. >>>> my @flags; >>>>=20 >>>> - # Init libloc database handle. >>>> - my $db_handle =3D &init(); >>>> - >>>> # Loop through the hash of possible network flags. >>>> foreach my $flag (keys(%network_flags)) { >>>> # Check if the address has the current flag. >>>> -- >>>> 2.20.1 --===============3410627596928742955==--