You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by zw...@apache.org on 2022/04/05 19:11:51 UTC

[trafficserver] branch 9.2.x updated: Lua plugin memory leak on remap configuration reloads (#8764)

This is an automated email from the ASF dual-hosted git repository.

zwoop pushed a commit to branch 9.2.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git


The following commit(s) were added to refs/heads/9.2.x by this push:
     new 4d05e9bd9 Lua plugin memory leak on remap configuration reloads (#8764)
4d05e9bd9 is described below

commit 4d05e9bd9f79e7e1c1c7376e7726926abe2a8bf4
Author: pbchou <pb...@labs.att.com>
AuthorDate: Wed Mar 30 08:42:36 2022 -0700

    Lua plugin memory leak on remap configuration reloads (#8764)
    
    This fix adds reference counting for the Lua plugin remap instance handles. The reference counting
    allows us to eliminate an existing memory leak of the instance handles. In addition, this means
    that the old Lua memory allocated by LuaJIT may also be freed via LuaJIT garbage collection.
    
    This fix also adds the '--ljgc' remap instance plugin parameter to the Lua plugin. This paramter
    enables on-demand LuaJIT garbage collection while the remap instances are created and deleted.
    This is useful when operating close to the LuaJIT memory limit, which is currently 2GB on Linux
    using LuaJIT v2.1.0-beta3 from 2017.
    
    Fixes #8728
    
    (cherry picked from commit b6f83f15dd5da3f09b5a851933c2a6feb5ec9488)
---
 doc/admin-guide/plugins/lua.en.rst | 24 +++++++++++++++++++++++-
 plugins/lua/ts_lua.c               | 23 ++++++++++++++++++++---
 plugins/lua/ts_lua_common.h        |  2 ++
 plugins/lua/ts_lua_util.c          | 33 +++++++++++++++++++++++++++++++--
 4 files changed, 76 insertions(+), 6 deletions(-)

diff --git a/doc/admin-guide/plugins/lua.en.rst b/doc/admin-guide/plugins/lua.en.rst
index 8da1cdea9..dd5f54ca3 100644
--- a/doc/admin-guide/plugins/lua.en.rst
+++ b/doc/admin-guide/plugins/lua.en.rst
@@ -136,7 +136,29 @@ adding a configuration option to records.config.
 
     CONFIG proxy.config.plugin.lua.max_states INT 64
 
-Any per plugin --states value overrides this default value but must be less than or equal to this value.  This setting is not reloadable since it must be applied when all the lua states are first initialized.
+Any per plugin --states value overrides this default value but must be less than or equal to this value.  This setting is not
+reloadable since it must be applied when all the lua states are first initialized.
+
+For remap instances, the LuaJIT garbage collector can be set to be called automatically whenever a remap instance is created
+or deleted. This happens when the remap.config file has been modified, and the configuration has been reloaded.  This does
+not apply to global plugin instances since these exist for the life-time of the ATS process, i.e., they are not reloadable or
+reconfigurable by modifying plugin.config while ATS is running.
+
+By default, the LuaJIT garbage collector will run on its own according to its own internal criteria.  However, in some cases,
+the garbage collector should be run in a guaranteed fashion.
+
+For example, in Linux, total Lua memory may be limited to 2GB depending on the LuaJIT version. It may be required to release
+memory on demand in order to prevent out of memory errors when running close to the memory limit. Note that the memory usage
+is doubled during configuration reloads since the ATS must hold both the current and new configurations during the
+transition. If garbage collection occurs does not occur immediately, memory usage may exceed this double usage.
+
+On demand garbage collection can be enabled by adding the following to each remap line. A value of '1' means
+enabled. The default value of '0' means disabled.
+
+::
+
+    map http://a.tbcdn.cn/ http://inner.tbcdn.cn/ @plugin=/XXX/tslua.so @pparam=--ljgc=1
+
 
 Profiling
 =========
diff --git a/plugins/lua/ts_lua.c b/plugins/lua/ts_lua.c
index b9e4bd358..8e609fd5a 100644
--- a/plugins/lua/ts_lua.c
+++ b/plugins/lua/ts_lua.c
@@ -342,9 +342,11 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char *errbuf, int errbuf_s
   char *inline_script                  = "";
   int fn                               = 0;
   int states                           = ts_lua_max_state_count;
+  int ljgc                             = 0;
   static const struct option longopt[] = {
     {"states", required_argument, 0, 's'},
     {"inline", required_argument, 0, 'i'},
+    {"ljgc", required_argument, 0, 'g'},
     {0, 0, 0, 0},
   };
 
@@ -363,6 +365,10 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char *errbuf, int errbuf_s
       break;
     case 'i':
       inline_script = optarg;
+      break;
+    case 'g':
+      ljgc = atoi(optarg);
+      break;
     }
 
     if (opt == -1) {
@@ -423,6 +429,10 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char *errbuf, int errbuf_s
     conf->states    = states;
     conf->remap     = 1;
     conf->init_func = 0;
+    conf->ref_count = 1;
+    conf->ljgc      = ljgc;
+
+    TSDebug(TS_LUA_DEBUG_TAG, "Reference Count = %d , creating new instance...", conf->ref_count);
 
     if (fn) {
       snprintf(conf->script, TS_LUA_MAX_SCRIPT_FNAME_LENGTH, "%s", script);
@@ -445,6 +455,9 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char *errbuf, int errbuf_s
       ts_lua_script_register(ts_lua_main_ctx_array[0].lua, conf->script, conf);
       TSMutexUnlock(ts_lua_main_ctx_array[0].mutexp);
     }
+  } else {
+    conf->ref_count++;
+    TSDebug(TS_LUA_DEBUG_TAG, "Reference Count = %d , reference existing instance...", conf->ref_count);
   }
 
   *ih = conf;
@@ -458,9 +471,13 @@ TSRemapDeleteInstance(void *ih)
   int states = ((ts_lua_instance_conf *)ih)->states;
   ts_lua_del_module((ts_lua_instance_conf *)ih, ts_lua_main_ctx_array, states);
   ts_lua_del_instance(ih);
-  // because we now reuse ts_lua_instance_conf / ih for remap rules sharing the same lua script
-  // we cannot safely free it in this function during the configuration reloads
-  // we therefore are leaking memory on configuration reloads
+  ((ts_lua_instance_conf *)ih)->ref_count--;
+  if (((ts_lua_instance_conf *)ih)->ref_count == 0) {
+    TSDebug(TS_LUA_DEBUG_TAG, "Reference Count = %d , freeing...", ((ts_lua_instance_conf *)ih)->ref_count);
+    TSfree(ih);
+  } else {
+    TSDebug(TS_LUA_DEBUG_TAG, "Reference Count = %d , not freeing...", ((ts_lua_instance_conf *)ih)->ref_count);
+  }
   return;
 }
 
diff --git a/plugins/lua/ts_lua_common.h b/plugins/lua/ts_lua_common.h
index d076ae13d..c39d03990 100644
--- a/plugins/lua/ts_lua_common.h
+++ b/plugins/lua/ts_lua_common.h
@@ -96,6 +96,8 @@ typedef struct {
 
   int remap;
   int states;
+  int ljgc;
+  int ref_count;
 
   int init_func;
 } ts_lua_instance_conf;
diff --git a/plugins/lua/ts_lua_util.c b/plugins/lua/ts_lua_util.c
index b9948d1ab..fa9685581 100644
--- a/plugins/lua/ts_lua_util.c
+++ b/plugins/lua/ts_lua_util.c
@@ -319,6 +319,13 @@ ts_lua_add_module(ts_lua_instance_conf *conf, ts_lua_main_ctx *arr, int n, int a
     lua_newtable(L);
     lua_replace(L, LUA_GLOBALSINDEX); /* L[GLOBAL] = EMPTY */
 
+    if (conf->ljgc > 0) {
+      TSDebug(TS_LUA_DEBUG_TAG, "ljgc = %d, running LuaJIT Garbage Collector...", conf->ljgc);
+      lua_gc(L, LUA_GCCOLLECT, 0);
+    } else {
+      TSDebug(TS_LUA_DEBUG_TAG, "ljgc = %d, NOT running LuaJIT Garbage Collector...", conf->ljgc);
+    }
+
     TSMutexUnlock(arr[i].mutexp);
   }
 
@@ -353,12 +360,27 @@ ts_lua_del_module(ts_lua_instance_conf *conf, ts_lua_main_ctx *arr, int n)
     }
 
     lua_pushlightuserdata(L, conf);
-    lua_pushvalue(L, LUA_GLOBALSINDEX);
-    lua_rawset(L, LUA_REGISTRYINDEX); /* L[REG][conf] = L[GLOBAL] */
+
+    if (conf->ref_count > 1) {
+      TSDebug(TS_LUA_DEBUG_TAG, "Reference Count = %d , NOT clearing registry...", conf->ref_count);
+      lua_pushvalue(L, LUA_GLOBALSINDEX);
+      lua_rawset(L, LUA_REGISTRYINDEX); /* L[REG][conf] = L[GLOBAL] */
+    } else {
+      TSDebug(TS_LUA_DEBUG_TAG, "Reference Count = %d , clearing registry...", conf->ref_count);
+      lua_pushnil(L);
+      lua_rawset(L, LUA_REGISTRYINDEX); /* L[REG][conf] = nil */
+    }
 
     lua_newtable(L);
     lua_replace(L, LUA_GLOBALSINDEX); /* L[GLOBAL] = EMPTY  */
 
+    if (conf->ljgc > 0) {
+      TSDebug(TS_LUA_DEBUG_TAG, "ljgc = %d, running LuaJIT Garbage Collector...", conf->ljgc);
+      lua_gc(L, LUA_GCCOLLECT, 0);
+    } else {
+      TSDebug(TS_LUA_DEBUG_TAG, "ljgc = %d, NOT running LuaJIT Garbage Collector...", conf->ljgc);
+    }
+
     TSMutexUnlock(arr[i].mutexp);
   }
 
@@ -420,6 +442,13 @@ ts_lua_reload_module(ts_lua_instance_conf *conf, ts_lua_main_ctx *arr, int n)
     lua_newtable(L);
     lua_replace(L, LUA_GLOBALSINDEX); /* L[GLOBAL] = EMPTY */
 
+    if (conf->ljgc > 0) {
+      TSDebug(TS_LUA_DEBUG_TAG, "ljgc = %d, running LuaJIT Garbage Collector...", conf->ljgc);
+      lua_gc(L, LUA_GCCOLLECT, 0);
+    } else {
+      TSDebug(TS_LUA_DEBUG_TAG, "ljgc = %d, NOT running LuaJIT Garbage Collector...", conf->ljgc);
+    }
+
     TSMutexUnlock(arr[i].mutexp);
   }