Conversation
| struct lua_State *L; | ||
| struct work_struct work; | ||
| spinlock_t lock; | ||
| bool init; |
|
|
||
| #ifdef CONFIG_XDP_LUA | ||
|
|
||
| static inline void verify_and_lock(void) { |
There was a problem hiding this comment.
Suggested change, by @lneto
static inline lua_State *xdplua_lock(void) {
struct xdplua_create_work *lw;
lw = this_cpu_ptr(&luaworks);
if (!lw->inuse) {
lw->inuse = true;
spin_lock(&lw->lock);
}
return lw->L;
}
There was a problem hiding this comment.
please, take care about {.. please review the whole patch..
| } | ||
| } | ||
|
|
||
| BPF_CALL_2(bpf_lua_dataref, struct xdp_buff *, ctx, int, offset) { |
There was a problem hiding this comment.
Suggested change, by @lneto
BPF_CALL_2(bpf_lua_dataref, struct xdp_buff *, ctx, int, offset) {
lua_State *L;
if (ctx->data_end <= offset + ctx->data)
return -1;
L = xdplua_lock();
return ldata_newref(L, ctx->data + offset,
ctx->data_end - ctx->data - offset);
}
There was a problem hiding this comment.
btw, I got your concern about moving such declarations to another file.. but we can solve this using some macro sorcery.. however, we can leave it as a TODO.. let's merge this PR first..
VictorNogueiraRio
force-pushed
the
vnogueira-5.6_nolimit
branch
from
bdd9779 to
2eebe24
Compare
|
|
||
| #ifdef CONFIG_XDP_LUA | ||
| struct xdplua_create_work { | ||
| char lua_script[8192]; |
There was a problem hiding this comment.
s/8192/MAXSCRIPTLEN/
There was a problem hiding this comment.
XDPLUA_MAX_SCRIPT_LEN or XDPLUA_MAXSCRIPTLEN? any variation looks good for me.. 👍
| void *data_hard_start; | ||
| unsigned long handle; | ||
| struct xdp_rxq_info *rxq; | ||
| #ifdef CONFIG_XDP_LUA |
There was a problem hiding this comment.
With the creation of struct xdplua_create_work there probably is no reason to modify struct xdp_buff
| } ____cacheline_aligned; /* perf critical, avoid false-sharing */ | ||
|
|
||
| #ifdef CONFIG_XDP_LUA | ||
| struct xdplua_create_work { |
There was a problem hiding this comment.
Maybe we should add a field of luadata's datarefs into this struct to automate the reference control of these objects
There was a problem hiding this comment.
@VictorNogueiraRio it makes sense to me.. also, we should rename this struct.. perhaps, xdplua_work.. create_work sounds like a function..
| lw->L = luaL_newstate(); | ||
| WARN_ON(!lw->L); | ||
|
|
||
| if (!lw->L) |
There was a problem hiding this comment.
We need to improve this, @lneto any suggestions?
There was a problem hiding this comment.
IMHO, we should fail XDPLua initiation completely and destroy all states we have created so far.. and, of course, disable XDPLua execution in runtime putting a guard..
There was a problem hiding this comment.
btw, another example of what I was arguing on slack is to move all this code to a xdplua_init() function
|
|
||
| for_each_possible_cpu(cpu) { | ||
| lw = per_cpu_ptr(&luaworks, cpu); | ||
| strcpy(lw->lua_script, lua_script); |
There was a problem hiding this comment.
strncpy(lw->lua_script, lua_script, MAXSCRIPTLEN);
| [IFLA_XDP_FLAGS] = { .type = NLA_U32 }, | ||
| [IFLA_XDP_PROG_ID] = { .type = NLA_U32 }, | ||
| #ifdef CONFIG_XDP_LUA | ||
| [IFLA_XDP_LUA_PROG] = { .type = NLA_STRING, .len = 8192 }, |
There was a problem hiding this comment.
s/8192/MAXSCRIPTLEN
include/uapi/linux/bpf.h
Outdated
| * 1 if the value at the given index of the Lua stack is a | ||
| * string; otherwise it returns 0 | ||
| * | ||
| * void bpf_lua_type(void *ctx, int index) |
tools/include/uapi/linux/bpf.h
Outdated
| * 1 if the value at the given index of the Lua stack is a | ||
| * string; otherwise it returns 0 | ||
| * | ||
| * void bpf_lua_type(void *ctx, int index) |
| int generic_xdp_lua_install_prog(char *lua_script) | ||
| { | ||
| int cpu; | ||
| struct xdplua_create_work *lw; |
There was a problem hiding this comment.
this variable can be moved to the for_each scope, right?
| #ifdef CONFIG_XDP_LUA | ||
| #include <lua.h> | ||
| #include <luadata.h> | ||
| #include <luaunpack.h> |
There was a problem hiding this comment.
one example of what I was arguing on slack is to have one single header to xdplua, e.g., xdplua.h
| if (lua_isstring(ctx->L, index)) { | ||
| strncpy(str, lua_tostring(ctx->L, index), size); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
let's handle the exception case first.. suggestion:
if (!lua_isstring(ctx->L, index))
return 0;
strncpy(str, lua_tostring(ctx->L, index), size);
return 1;
There was a problem hiding this comment.
@VictorNogueiraRio what's the use case, if we have none, I would simply remove this helper. At least, I would rename it to something like bpf_lua_copystring. Also, I believe you should pop the string out of the stack, right?
| case BPF_FUNC_jiffies64: | ||
| return &bpf_jiffies64_proto; | ||
| #ifdef CONFIG_XDP_LUA | ||
| case BPF_FUNC_lua_dataref: |
There was a problem hiding this comment.
we could automate it using a macro.. e.g., XDP_LUA_BPF_FUNC(lua_dataref);
| goto errout; | ||
| status |= DO_SETLINK_NOTIFY; | ||
| } | ||
|
|
| goto errout; | ||
| } | ||
| #endif /* CONFIG_XDP_LUA */ | ||
|
|
|
|
||
| function update(map) | ||
| xdp.map_update(map, 1, 3) | ||
| end |
| return XDP_PASS; | ||
| } | ||
|
|
||
| char _license[] SEC("license") = "GPL"; |
| end | ||
|
|
||
| return cookies[ip] == cookiepkt and true or false | ||
| end |
| return parse_tcp(ctx, data, ip_off, data_end, saddr); | ||
| } | ||
|
|
||
| char _license[] SEC("license") = "GPL"; |
|
|
||
| if (monitor) { | ||
| rx_cnt_map_fd = bpf_object__find_map_fd_by_name(obj, "rx_cnt"); | ||
|
|
| } | ||
| } | ||
|
|
||
|
|
| * | ||
| * int bpf_lua_dataref(void *ctx, int offset) | ||
| * Description | ||
| * Create a new lua data buffer object pointing to the captured |
There was a problem hiding this comment.
| * Create a new lua data buffer object pointing to the captured | |
| * Creates a new lua data object pointing to the captured |
| * packet at the specified offset. The function leaves the new | ||
| * object on top of the Lua stack. | ||
| * Return | ||
| * Data object reference number on success, or -1 in case |
There was a problem hiding this comment.
| * Data object reference number on success, or -1 in case | |
| * Data object reference number on success, or LUA_NOREF in case |
| .arg2_type = ARG_ANYTHING, | ||
| }; | ||
|
|
||
| BPF_CALL_4(bpf_lua_pcall, struct xdp_buff *, ctx, char *, funcname, |
There was a problem hiding this comment.
@VictorNogueiraRio I believed we have already discussed that we should just wrap lua_pcall and allow the caller to handle the errors..
|
|
||
| base += num_rets; | ||
|
|
||
| clean_state: |
| BPF_CALL_2(bpf_lua_pushmap, struct xdp_buff *, ctx, struct bpf_map *, map) { | ||
| verify_and_lock(); | ||
| lua_pushlightuserdata(ctx->L, map); | ||
| return 0; |
There was a problem hiding this comment.
do we always need to return int even if we want a void?
| return 0; | ||
| } | ||
|
|
||
| static const struct bpf_func_proto bpf_lua_pushlstring_proto = { |
There was a problem hiding this comment.
I really believe we can have a macro to generate the wrapper.. including such struct..
P.S.: we can leave it as a TODO, of course..
| }; | ||
|
|
||
| BPF_CALL_2(bpf_lua_type, struct xdp_buff *, ctx, int, index) { | ||
| return lua_type(ctx->L, index); |
| * Converts the Lua value at the given index of the Lua | ||
| * stack to a C string and copies size bytes of it to | ||
| * value pointed by str | ||
| * Return |
| * following constants defined in lua.h: LUA_TNIL (0), | ||
| * LUA_TNUMBER, LUA_TBOOLEAN, LUA_TSTRING, LUA_TTABLE, | ||
| * LUA_TFUNCTION, LUA_TUSERDATA, LUA_TTHREAD, and | ||
| * LUA_TLIGHTUSERDATA. |
There was a problem hiding this comment.
we don't need this long description.. you can just refer to the manual or lua.h itself as you're doing already..
| * Pushes the zero-terminated string pointed to by s onto the stack. | ||
| * Lua makes (or reuses) an internal copy of the given string, | ||
| * so the memory at s can be freed or reused immediately after the | ||
| * function returns. |
There was a problem hiding this comment.
we don't need to replicate Lua's manual here.. just refer to it..
There was a problem hiding this comment.
this comment refers to all wrappers.. please, review all..
| * | ||
| * void bpf_lua_pushskb(void *ctx) | ||
| * Description | ||
| * Pushes an SKB structure onto the Lua stack |
|
|
||
| #ifdef CONFIG_XDP_LUA | ||
| if (xdp[IFLA_XDP_LUA_PROG]) { | ||
| char *lua_prog = nla_data(xdp[IFLA_XDP_LUA_PROG]); |
There was a problem hiding this comment.
script? I think we should uniformize it.. we are using both lua_prog and lua_script in the code.. I would rename them all to script
lneto
left a comment
lneto
left a comment
There was a problem hiding this comment.
I think moving the code from dev.c, filter.c and rtnetlink.c to a xdplua.c would cleanup the patch considerably.
marcelstanley
left a comment
marcelstanley
left a comment
There was a problem hiding this comment.
Just adding a minor request. I still need to do a thorough review.
|
|
||
| lua_prog = (char *) malloc(prog_size + 1); | ||
| memset(lua_prog, 0, prog_size + 1); | ||
| if (fread(lua_prog, 1, prog_size, f) < 0) { |
There was a problem hiding this comment.
From #3
| if (fread(lua_prog, 1, prog_size, f) < 0) { | |
| size_t read = fread(lua_prog, 1, prog_size, f); | |
| if (read < 0) { |
3 participants
No description provided.