From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bernhard Bitsch To: development@lists.ipfire.org Subject: Aw: Re: [PATCH 1/5] location-functions.pl: Use a single script-wide db_handle. Date: Mon, 09 Nov 2020 16:16:22 +0100 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5726622321644289859==" List-Id: --===============5726622321644289859== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > Gesendet: Montag, 09. November 2020 um 15:04 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 db= _handle. > > Hello, >=20 > > 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= db_handle. > >>=20 > >> Hello, > >>=20 > >> Thank you Stefan for submitting this patchset. > >>=20 > >>> On 8 Nov 2020, at 19:36, Bernhard Bitsch wro= te: > >>>=20 > >>> This means, we stay with the unbalanced memory allocation in (Perl) lib= loc. Which leaves a memory leak. > >>=20 > >> Bernhard, could you please elaborate on how this memory leak is still ex= isting? > >>=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 = once and the handle is being stored internally. All functions that are being = called will no longer have to hold their own database handle. Therefore the m= aximum amount of handles open is one. > >>=20 > >=20 > > The init() function allocates the handle, which is not really destroyed. = The END block is just a safety process. The allocated memory for the handle s= hould be released in the moment of references=3D=3D0. >=20 > The handle is being initialised when location-functions.pl is being loaded = and it is freed when the Perl interpreter ends. >=20 > It remains referenced all the time. >=20 This the new state! The init() function builds a memory block to access the location data base an= d returns a handle to it. This memory block is not released by the memory man= agement. I suppose there are some deficiencies in handling the reference coun= ters. A call to init() in each function of Location::Functions leaves this memory b= lock. That was the originatings issue. > > Yes, the new code ( inspired by my work-around ) minimizes the number of = handles to one. But this handle is persistent also ( this doesn't hurt becaus= e it is used persistent for the time of module use ). >=20 > It is only being initialised when location-functions.pl is being loaded. Th= at only happens in code that uses those functions. >=20 > I do not see the problem with this. Constantly closing and re-opening the d= atabase is not an option since the connection tracking list can have tens of = thousands of lookups and this will make the page - simply - slow. >=20 > I cannot see an option that is more resource-friendly than this. >=20 It is not the open/close of the data base which produces the memory leak, but= the Perl interface ( see above ).=20 In terms of performance, you are right. So my "mini" system showed as the way= to a better performing system. > >=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 d= b_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 i= n 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 re= ading the code? > >>=20 > >> Best, > >> -Michael > >>=20 > >=20 > > The sentence about "small systems" refers to the original comment. It is = not a real memory saving, the patch cures a 'memory wasting'. It is clear, th= at a massively use of malloc without free crashes a small system faster than = a system with big memory resources. >=20 > Opening the database requires pretty much nothing (maybe 4K) of RSS memory = and about 50MB of VIRT memory. This due to mmap() which helps us reading the = database a lot fast. I think this is very very reasonable. The more data is b= eing read from the database the more memory it will use. >=20 > If you wish to change that (and that comes with a performance penalty) you = could submit a patch that removes mmap() if the user wishes to. >=20 > 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. >=20 > We have found the most resource-saving way to deal with this now and anythi= ng else will simply use considerable amount of time with no benefit to 99% of= our users. >=20 > Best, > -Michael >=20 Further I think, we have found the most straight-forward way to interface to = libloc ( which is a great work! ). The first approach remembered me a bit at = good old times when M$ took MS-DOS and some code upon and called it Windows. = More and more layers put on this building requested faster processors and mor= e memory, without real gain in functionality. Just the experience of an "old"= computer scientist, which saw each step in this evolution. ;) Best, -Bernhard > > The patchset is mainly my work-around, thus it is in test since I found t= he 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/loc= ation-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, $addr= ess); > >>>> @@ -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 >=20 > --===============5726622321644289859==--