Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: added support for ARM64. #1379

Closed
wants to merge 4 commits into from

Conversation

spacewander
Copy link
Member

now we require OpenResty's LuaJIT branch to work properly on ARM64 since
we make use of the thread.exdata Lua API.

On architectures other than ARM64, the thread.exdata API also saves the
per-request global env table and closure objects for each light thread,
which gives a nice ~10% speedup for the simplest Lua handler location
loaded by wrk over HTTP 1.1.

We now use proper userdata instead of lightuserdata for the
ngx.shared.DICT Lua objects.

we also mask off the bits higher than 47-bit of lightuserdata keys based
on C static variable addresses.

We also implemented the pure C API for the ndk.* API (to be used in
lua-resty-core).

Thanks Dejiang Zhu and Zexuan Luo for the development work of this patch.

Signed-off-by: Yichun Zhang (agentzh) agentzh@gmail.com

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

now we require OpenResty's LuaJIT branch to work properly on ARM64 since
we make use of the thread.exdata Lua API.

On architectures other than ARM64, the thread.exdata API also saves the
per-request global env table and closure objects for each light thread,
which gives a nice ~10% speedup for the simplest Lua handler location
loaded by wrk over HTTP 1.1.

We now use proper userdata instead of lightuserdata for the
ngx.shared.DICT Lua objects.

we also mask off the bits higher than 47-bit of lightuserdata keys based
on C static variable addresses.

We also implemented the pure C API for the ndk.* API (to be used in
lua-resty-core).

Thanks Dejiang Zhu and Zexuan Luo for the development work of this patch.

Signed-off-by: Yichun Zhang (agentzh) <agentzh@gmail.com>
@javierguerragiraldez
Copy link

we've tested this at Cloudflare, seems stable enough.

@@ -213,6 +217,24 @@ struct ngx_http_lua_main_conf_s {
* thus it is safe to store the peer data in the main conf.
*/

ngx_chain_t *body_filter_chain;
/* body_filter_by_lua does not support yielding and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use body_filter_by_lua* instead to cover other variants. Please address other similar places.

*/

ngx_http_variable_value_t *setby_args;
/* set_by_lua does not support yielding and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

*/

size_t setby_nargs;
/* set_by_lua does not support yielding and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.


#ifdef OPENRESTY_LUAJIT
{
const char buf[] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should put the int rc declaration here as well. rc is not used elsewhere and we should limit its lifetime.

"failed to run Lua code for _G write guard: %s",
rc, lua_tostring(L, -1));
lua_pop(L, 1);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 if statements should be in the same block as the buf variable.

t/014-bugs.t Outdated
--- response_body eval
["0", "0", "0"]
--- response_body_like eval
[qr/^(0|3|6)$/, qr/^(1|4|7)$/, qr/^(2|5|8)$/]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not use capturing groups in such regexes. We should use (?: ) instead to avoid useless submatch capturing. Please address other similar places in the tests. Also, it seems that it's simpler to just use [036] instead of (?:0|3|6) here, for example?

t/014-bugs.t Outdated
--- response_body eval
["0", "0", "0"]
--- response_body_like eval
[qr/^(0|3|6)$/, qr/^(1|4|7)$/, qr/^(2|5|8)$/]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

--- no_error_log
[error]
--- grep_error_log eval: qr/old foo: \d+/
--- grep_error_log_out eval
["", "old foo: 1\n"]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should generate write guard warnings. Let's check for them in these tests writing to globals in request contexts.

Copy link
Member

@agentzh agentzh Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should cover the log prefix "http lua attempting to" here as well. Also we should check if the error log level is warn. The current pattern does not cover these bits.

I think we could use --- error_log here instead for checking the warning log message?

"if key_type == 'string' then\n"
"ngx_log(ngx_WARN, 'lua http setting global variable, "
"key: ', key, "
"', value type: ', type(value), '\\n', "
Copy link
Member

@agentzh agentzh Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message is not very intuitive. key does not make much sense to the end users since they may just write to a global variable, not invoking the metamethod explicitly via a key. Something like below would be much clearer:

lua http attempting to write to the lua global variable 'foo'

It's not very useful to log the type of the value IMHO. Let's make it concise.

Also, it seems that there's no test cases covering this warning message at all?

"ngx_log(ngx_WARN, 'lua http setting global variable, "
"key type: ', key_type, "
"', value type: ', type(value), '\\n', "
"traceback())\n"
Copy link
Member

@agentzh agentzh Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like above, we should log something like this:

lua http attempting to write to the lua global variable 'table: 0x40e6ca60'

So we can actually merge these 2 code branches and just use tostring(key) instead. It works on both the string-typed variable name and other types.

@spacewander
Copy link
Member Author

@agentzh
Updated.

"if phase == 'init_worker' or phase == 'init' then\n"
"return\n"
"end\n"
"ngx_log(ngx_WARN, 'lua http attempting to write to the lua "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, after some more thinking, it's better to use the prefix "http lua" instead? So basically the log message prefix could be

<subsys> <module>

So we should use "stream lua" in the ngx_stream_lua_module.

--- no_error_log
[error]
--- grep_error_log eval: qr/old foo: \d+/
--- grep_error_log_out eval
["", "old foo: 1\n"]

Copy link
Member

@agentzh agentzh Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should cover the log prefix "http lua attempting to" here as well. Also we should check if the error log level is warn. The current pattern does not cover these bits.

I think we could use --- error_log here instead for checking the warning log message?

qr/[12]/
--- grep_error_log eval: qr/(old foo: \d+|write to the lua global variable '\w+')/
--- grep_error_log_out eval
["write to the lua global variable 'foo'\n", "old foo: 1\n"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should cover the log level (warn) and the Lua backtrace (is there a backtrace logged at all?).

echo $res;
}
--- response_body_like eval
qr/[12]/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regex is too loose...Output like hello, 1234 will also match this regex happily.

@agentzh
Copy link
Member

agentzh commented Oct 18, 2018

Merged with the following extra patch:

diff --git a/.travis.yml b/.travis.yml
index 01a2d8fb..8c8e3a75 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -72,7 +72,7 @@ install:
   - git clone https://github.com/openresty/no-pool-nginx.git ../no-pool-nginx
   - git clone https://github.com/openresty/openresty-devel-utils.git
   - git clone https://github.com/openresty/mockeagain.git
-  - git clone -b arm64 https://github.com/spacewander/lua-cjson.git lua-cjson
+  - git clone https://github.com/openresty/lua-cjson.git lua-cjson
   - git clone https://github.com/openresty/lua-upstream-nginx-module.git ../lua-upstream-nginx-module
   - git clone https://github.com/openresty/echo-nginx-module.git ../echo-nginx-module
   - git clone https://github.com/openresty/nginx-eval-module.git ../nginx-eval-module
@@ -85,11 +85,11 @@ install:
   - git clone https://github.com/openresty/rds-json-nginx-module.git ../rds-json-nginx-module
   - git clone https://github.com/openresty/srcache-nginx-module.git ../srcache-nginx-module
   - git clone https://github.com/openresty/redis2-nginx-module.git ../redis2-nginx-module
-  - git clone -b arm64 https://github.com/spacewander/lua-resty-core.git ../lua-resty-core
+  - git clone https://github.com/openresty/lua-resty-core.git ../lua-resty-core
   - git clone https://github.com/openresty/lua-resty-lrucache.git ../lua-resty-lrucache
   - git clone https://github.com/openresty/lua-resty-mysql.git ../lua-resty-mysql
-  - git clone -b arm64 https://github.com/spacewander/stream-lua-nginx-module.git ../stream-lua-nginx-module
-  - git clone -b exdata https://github.com/spacewander/luajit2.git luajit2
+  - git clone https://github.com/openresty/stream-lua-nginx-module.git ../stream-lua-nginx-module
+  - git clone -b v2.1-agentzh https://github.com/openresty/luajit2.git luajit2
 
 before_script:
   - mysql -uroot -e 'create database ngx_test; grant all on ngx_test.* to "ngx_test"@"%" identified by "ngx_test"; flush privileges;'
diff --git a/src/ngx_http_lua_common.h b/src/ngx_http_lua_common.h
index 5e6b26b3..5eb57c69 100644
--- a/src/ngx_http_lua_common.h
+++ b/src/ngx_http_lua_common.h
@@ -212,28 +212,31 @@ struct ngx_http_lua_main_conf_s {
     ngx_str_t                            init_worker_src;
 
     ngx_http_lua_balancer_peer_data_t      *balancer_peer_data;
-                    /* balancer_by_lua* does not support yielding and
-                     * there cannot be any conflicts among concurrent requests,
-                     * thus it is safe to store the peer data in the main conf.
+                    /* neither yielding nor recursion is possible in
+                     * balancer_by_lua*, so there cannot be any races among
+                     * concurrent requests and it is safe to store the peer
+                     * data pointer in the main conf.
                      */
 
     ngx_chain_t                            *body_filter_chain;
-                    /* body_filter_by_lua* does not support yielding/recursive
-                     * calling and there cannot be any conflicts among
-                     * concurrent requests, thus it is safe to store the
-                     * chain data in the main conf.
+                    /* neither yielding nor recursion is possible in
+                     * body_filter_by_lua*, so there cannot be any races among
+                     * concurrent requests when storing the chain
+                     * data pointer in the main conf.
                      */
 
     ngx_http_variable_value_t              *setby_args;
-                    /* set_by_lua* does not support yielding and
-                     * there cannot be any conflicts among concurrent requests,
-                     * thus it is safe to store the args point in the main conf.
+                    /* neither yielding nor recursion is possible in
+                     * set_by_lua*, so there cannot be any races among
+                     * concurrent requests when storing the args pointer
+                     * in the main conf.
                      */
 
     size_t                                  setby_nargs;
-                    /* set_by_lua* does not support yielding and
-                     * there cannot be any conflicts among concurrent requests,
-                     * thus it is safe to store the nargs in the main conf.
+                    /* neither yielding nor recursion is possible in
+                     * set_by_lua*, so there cannot be any races among
+                     * concurrent requests when storing the nargs in the
+                     * main conf.
                      */
 
     ngx_uint_t                      shm_zones_inited;
diff --git a/src/ngx_http_lua_util.c b/src/ngx_http_lua_util.c
index 12ef186b..c6e21f62 100644
--- a/src/ngx_http_lua_util.c
+++ b/src/ngx_http_lua_util.c
@@ -780,7 +780,7 @@ ngx_http_lua_inject_ngx_api(lua_State *L, ngx_http_lua_main_conf_t *lmcf,
                 "if phase == 'init_worker' or phase == 'init' then\n"
                     "return\n"
                 "end\n"
-                "ngx_log(ngx_WARN, 'lua http attempting to write to the lua "
+                "ngx_log(ngx_WARN, 'http lua attempting to write to the lua "
                                    "global variable \\'', tostring(key), "
                                    "'\\'\\n', traceback())\n"
             "end\n"
diff --git a/t/143-ssl-session-fetch.t b/t/143-ssl-session-fetch.t
index da45aa34..78a66cf9 100644
--- a/t/143-ssl-session-fetch.t
+++ b/t/143-ssl-session-fetch.t
@@ -1210,8 +1210,8 @@ qr/ssl_session_fetch_by_lua_block:1: ssl fetch sess by lua is running!/s
 
 --- request
 GET /t
---- response_body_like eval
-qr/^(1|2|3)done$/
+--- response_body_like chomp
+\A[123]done\n\z
 --- grep_error_log eval: qr/old (foo|bar): \d+/
 --- grep_error_log_out eval
 ["", "old foo: 1\n", "old bar: 1\nold foo: 2\n"]
diff --git a/t/157-global-var.t b/t/157-global-var.t
index 69fed4fd..3c4f843f 100644
--- a/t/157-global-var.t
+++ b/t/157-global-var.t
@@ -57,11 +57,13 @@ __DATA__
         }
         echo $res;
     }
---- response_body_like eval
-qr/[12]/
---- grep_error_log eval: qr/(old foo: \d+|write to the lua global variable '\w+')/
+--- response_body_like chomp
+\A[12]\n\z
+--- grep_error_log eval
+qr/(old foo: \d+|\[\w+\].*?http lua attempting to write to the lua global variable '[^'\s]+'|set_by_lua:\d+: in main chunk, )/
 --- grep_error_log_out eval
-["write to the lua global variable 'foo'\n", "old foo: 1\n"]
+[qr/\A\[warn\] .*?http lua attempting to write to the lua global variable 'foo'
+set_by_lua:3: in main chunk, \n\z/, "old foo: 1\n"]
 
 
 
@@ -78,11 +80,13 @@ qr/[12]/
             ngx.say(foo)
         }
     }
---- response_body_like eval
-qr/[12]/
---- grep_error_log eval: qr/(old foo: \d+|write to the lua global variable '\w+')/
+--- response_body_like chomp
+\A[12]\n\z
+--- grep_error_log eval
+qr/(old foo: \d+|\[\w+\].*?http lua attempting to write to the lua global variable '[^'\s]+'|\w+_by_lua\(.*?\):\d+: in main chunk, )/
 --- grep_error_log_out eval
-["write to the lua global variable 'foo'\n", "old foo: 1\n"]
+[qr/\A\[warn\] .*?http lua attempting to write to the lua global variable 'foo'
+rewrite_by_lua\(nginx\.conf:48\):3: in main chunk, \n\z/, "old foo: 1\n"]
 
 
 
@@ -99,11 +103,13 @@ qr/[12]/
             ngx.say(foo)
         }
     }
---- response_body_like eval
-qr/[12]/
---- grep_error_log eval: qr/(old foo: \d+|write to the lua global variable '\w+')/
+--- response_body_like chomp
+\A[12]\n\z
+--- grep_error_log eval
+qr/(old foo: \d+|\[\w+\].*?http lua attempting to write to the lua global variable '[^'\s]+'|\w+_by_lua\(.*?\):\d+: in main chunk, )/
 --- grep_error_log_out eval
-["write to the lua global variable 'foo'\n", "old foo: 1\n"]
+[qr/\A\[warn\] .*?http lua attempting to write to the lua global variable 'foo'
+access_by_lua\(nginx\.conf:48\):3: in main chunk, \n\z/, "old foo: 1\n"]
 
 
 
@@ -120,11 +126,13 @@ qr/[12]/
             ngx.say(foo)
         }
     }
---- response_body_like eval
-qr/[12]/
---- grep_error_log eval: qr/(old foo: \d+|write to the lua global variable '\w+')/
+--- response_body_like chomp
+\A[12]\n\z
+--- grep_error_log eval
+qr/(old foo: \d+|\[\w+\].*?http lua attempting to write to the lua global variable '[^'\s]+'|\w+_by_lua\(.*?\):\d+: in main chunk, )/
 --- grep_error_log_out eval
-["write to the lua global variable 'foo'\n", "old foo: 1\n"]
+[qr/\A\[warn\] .*?http lua attempting to write to the lua global variable 'foo'
+content_by_lua\(nginx\.conf:48\):3: in main chunk, \n\z/, "old foo: 1\n"]
 
 
 
@@ -143,11 +151,13 @@ qr/[12]/
             end
         }
     }
---- response_body_like eval
-qr/^(nil|1)$/
---- grep_error_log eval: qr/(old foo: \d+|write to the lua global variable '\w+')/
+--- response_body_like chomp
+\A(?:nil|1)\n\z
+--- grep_error_log eval
+qr/(old foo: \d+|\[\w+\].*?http lua attempting to write to the lua global variable '[^'\s]+'|\w+_by_lua:\d+: in main chunk, )/
 --- grep_error_log_out eval
-["write to the lua global variable 'foo'\n", "old foo: 1\n"]
+[qr/\A\[warn\] .*?http lua attempting to write to the lua global variable 'foo'
+header_filter_by_lua:3: in main chunk, \n\z/, "old foo: 1\n"]
 
 
 
@@ -166,13 +176,14 @@ qr/^(nil|1)$/
             end
         }
     }
---- response_body_like eval
-qr/^(nil|2)$/
---- grep_error_log eval: qr/(old foo: \d+|write to the lua global variable '\w+')/
+--- response_body_like chomp
+\A(?:nil|2)\n\z
+--- grep_error_log eval
+qr/(old foo: \d+|\[\w+\].*?http lua attempting to write to the lua global variable '[^'\s]+'|\w+_by_lua:\d+: in main chunk, )/
 --- grep_error_log_out eval
-["write to the lua global variable 'foo'
-old foo: 1\n", "old foo: 2\nold foo: 3\n"]
-
+[qr/\[warn\] .*?http lua attempting to write to the lua global variable 'foo'
+body_filter_by_lua:3: in main chunk, 
+old foo: 1\n\z/, "old foo: 2\nold foo: 3\n"]
 
 
 === TEST 7: log_by_lua
@@ -190,11 +201,14 @@ old foo: 1\n", "old foo: 2\nold foo: 3\n"]
             end
         }
     }
---- response_body_like eval
-qr/^(nil|1)$/
---- grep_error_log eval: qr/(old foo: \d+|write to the lua global variable '\w+')/
+--- response_body_like chomp
+\A(?:nil|1)\n\z
+--- grep_error_log eval
+qr/(old foo: \d+|\[\w+\].*?http lua attempting to write to the lua global variable '[^'\s]+'|\w+_by_lua\(.*?\):\d+: in main chunk)/
 --- grep_error_log_out eval
-["write to the lua global variable 'foo'\n", "old foo: 1\n"]
+[qr/\A\[warn\] .*?http lua attempting to write to the lua global variable 'foo'
+log_by_lua\(nginx\.conf:50\):3: in main chunk\n\z/, "old foo: 1\n"]
+
 
 
 
@@ -280,11 +294,14 @@ qr/^(nil|1)$/
         }
     }
 
---- response_body_like eval
-qr/^(1|2)done$/
---- grep_error_log eval: qr/(old foo: \d+|write to the lua global variable '\w+')/
+--- response_body_like chomp
+\A[12]done\n\z
+--- grep_error_log eval
+qr/(old foo: \d+|\[\w+\].*?http lua attempting to write to the lua global variable '[^'\s]+'|\w+_by_lua:\d+: in main chunk)/
 --- grep_error_log_out eval
-["write to the lua global variable 'foo'\n", "old foo: 1\n"]
+[qr/\A\[warn\] .*?http lua attempting to write to the lua global variable 'foo'
+ssl_certificate_by_lua:3: in main chunk\n\z/, "old foo: 1\n"]
+
 
 
 
@@ -309,11 +326,14 @@ qr/^(1|2)done$/
             ngx.say(foo)
         }
     }
---- response_body_like eval
-qr/^(1|2)$/
---- grep_error_log eval: qr/(old foo: \d+|write to the lua global variable '\w+')/
+--- response_body_like chomp
+\A[12]\n\z
+--- grep_error_log eval
+qr/(old foo: \d+|\[\w+\].*?http lua attempting to write to the lua global variable '[^'\s]+'|\w+_by_lua\(.*?\):\d+: in\b)/
 --- grep_error_log_out eval
-["write to the lua global variable 'foo'\n", "old foo: 1\n"]
+[qr/\A\[warn\] .*?http lua attempting to write to the lua global variable 'foo'
+content_by_lua\(nginx\.conf:56\):4: in\n\z/, "old foo: 1\n"]
+
 
 
 
@@ -334,8 +354,8 @@ qr/^(1|2)$/
             ngx.say(foo)
         }
     }
---- response_body_like eval
-qr/^(2|3)$/
+--- response_body_like chomp
+\A[23]\n\z
 --- grep_error_log eval: qr/old foo: \d+/
 --- grep_error_log_out eval
 ["old foo: 1\n", "old foo: 2\n"]
@@ -364,8 +384,8 @@ qr/^(2|3)$/
             ngx.say(foo)
         }
     }
---- response_body_like eval
-qr/^(2|3)$/
+--- response_body_like chomp
+\A[23]\n\z
 --- grep_error_log eval: qr/old foo: \d+/
 --- grep_error_log_out eval
 ["old foo: 1\n", "old foo: 2\n"]
@@ -402,8 +422,8 @@ qr/^(2|3)$/
             ngx.say(foo)
         }
     }
---- response_body_like eval
-qr/^(3|4)$/
+--- response_body_like chomp
+\A[34]\n\z
 --- grep_error_log eval: qr/old foo: \d+/
 --- grep_error_log_out eval
 ["old foo: 1\nold foo: 2\n", "old foo: 3\n"]
@@ -455,9 +475,10 @@ setting global variable
             ngx.say(foo)
         }
     }
---- response_body_like eval
-qr/^(1|2)$/
---- grep_error_log eval: qr/(old foo: \d+|write to the lua global variable '\w+')/
+--- response_body_like chomp
+\A[12]\n\z
+--- grep_error_log eval
+qr/(old foo: \d+|write to the lua global variable '\w+')/
 --- grep_error_log_out eval
 ["write to the lua global variable 'foo'\n", "old foo: 1\n"]
 
@@ -482,8 +503,8 @@ qr/^(1|2)$/
     }
 --- response_body_like: 502 Bad Gateway
 --- error_code: 502
---- error_log
-connect() to 0.0.0.1:80 failed
+--- error_log eval
+qr/\[crit\].*?\Qconnect() to 0.0.0.1:80 failed\E/
 --- grep_error_log eval: qr/(old foo: \d+|write to the lua global variable '\w+')/
 --- grep_error_log_out eval
 ["write to the lua global variable 'foo'\n", "old foo: 1\n"]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants