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 17:03:38 +0000 Message-ID: <8295EB2E-D024-4DE3-BE7A-28146E9768E7@ipfire.org> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8022447150460008971==" List-Id: --===============8022447150460008971== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > On 9 Nov 2020, at 15:16, Bernhard Bitsch wrote: >=20 > Hello, >=20 >> 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 d= b_handle. >>=20 >> 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 >=20 > This the new state! > The init() function builds a memory block to access the location data base = and returns a handle to it. This memory block is not released by the memory m= anagement. I suppose there are some deficiencies in handling the reference co= unters. Okay, where are those? Libloc internally uses reference counting and we have some unit tests to chec= k if those are working okay. If you know that we are handling this somewhere incorrectly, let me know and = we will be able to reduce the footprint of libloc even more. > A call to init() in each function of Location::Functions leaves this memory= block. That was the originatings issue. >=20 >>> 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. T= hat only happens in code that uses those functions. >>=20 >> I do not see the problem with this. Constantly closing and re-opening the = database 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 >=20 > It is not the open/close of the data base which produces the memory leak, b= ut the Perl interface ( see above ).=20 Okay, but where? In which function? > In terms of performance, you are right. So my "mini" system showed as the w= ay to a better performing system. >=20 >>>=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 = being 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 anyth= ing else will simply use considerable amount of time with no benefit to 99% o= f our users. >>=20 >> Best, >> -Michael >>=20 >=20 > Further I think, we have found the most straight-forward way to interface t= o libloc ( which is a great work! ). The first approach remembered me a bit a= t 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 m= ore memory, without real gain in functionality. Just the experience of an "ol= d" computer scientist, which saw each step in this evolution. ;) To be totally honest, we wanted to keep the Perl interface as easy as possibl= e. The Python interface is much easier to integrate and will gain many more u= sers. Perl is on its way out, but of course the current web user interface is= written in perl. We started with only a few functions but it grew quickly an= d therefore it became messy. The native perl part is only there because the perl C bindings are less fun t= o use than putting hot wax into my eyes. -Michael >=20 > 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 --===============8022447150460008971==--