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.

Sign up for GitHub

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

Fix script loading synchronization issue and add LuaUnpack submodule #1

Open
wants to merge 16 commits into
base: lneto-1
Choose a base branch
from
+242 −121
Open

Fix script loading synchronization issue and add LuaUnpack submodule #1

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
ca0a05c
fixup! add XDPLua
VictorNogueiraRio Feb 11, 2020
00b981c
fixup! add LuaData submodule
VictorNogueiraRio Feb 11, 2020
2fda6a1
fixup! add XDPLua map sample
VictorNogueiraRio Feb 11, 2020
cd9fff9
fixup! fixup! add XDPLua
VictorNogueiraRio Feb 12, 2020
d29c749
fixup! add Lunatik submodule
VictorNogueiraRio Feb 13, 2020
aa6b663
Fix script loading synchronization issue
VictorNogueiraRio Feb 20, 2020
497fd52
add LuaUnpack submodule
VictorNogueiraRio Feb 20, 2020
42219e1
Add bpf_lua_tostring function
VictorNogueiraRio Feb 3, 2020
904bb0e
Add bpf_lua_type function
VictorNogueiraRio Mar 3, 2020
17af788
fixup! Add bpf_lua_tostring function
VictorNogueiraRio Mar 3, 2020
73e349a
fixup! Fix script loading synchronization issue
VictorNogueiraRio Mar 18, 2020
822408f
fixup! add XDPLua map sample
VictorNogueiraRio Mar 18, 2020
47d5469
fixup! add LuaRCU submodule
VictorNogueiraRio Mar 19, 2020
2bb260c
fixup! Fix script loading synchronization issue
VictorNogueiraRio Mar 23, 2020
5afe3af
fixup! Fix script loading synchronization issue
VictorNogueiraRio Apr 3, 2020
522f718
fixup! add XDPLua
VictorNogueiraRio Aug 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitmodules
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,6 @@
[submodule "lib/luarcu"]
path = lib/luarcu
url = https://github.com/luainkernel/luarcu
[submodule "lib/luaunpack"]
path = lib/luaunpack
url = https://github.com/VictorNogueiraRio/luaunpack.git
10 changes: 0 additions & 10 deletions include/linux/filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1229,14 +1229,4 @@ struct bpf_sockopt_kern {
s32 retval;
};

#ifdef CONFIG_XDPLUA
extern struct list_head lua_state_cpu_list;

struct lua_state_cpu {
struct lua_State *L;
int cpu;
struct list_head list;
};
#endif /* CONFIG_XDPLUA */

#endif /* __LINUX_FILTER_H__ */
4 changes: 2 additions & 2 deletions include/linux/netdevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3684,9 +3684,9 @@ u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
enum bpf_netdev_command cmd);
int xdp_umem_query(struct net_device *dev, u16 queue_id);

#ifdef CONFIG_XDPLUA
#ifdef CONFIG_XDP_LUA
int generic_xdp_lua_install_prog(char *lua_prog);
#endif /* CONFIG_XDPLUA */
#endif /* CONFIG_XDP_LUA */

int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
int dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
Expand Down
15 changes: 12 additions & 3 deletions include/net/xdp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,17 +63,26 @@ struct xdp_rxq_info {
struct xdp_mem_info mem;
} ____cacheline_aligned; /* perf critical, avoid false-sharing */

#ifdef CONFIG_XDP_LUA
struct xdplua {
struct lua_State *L;
spinlock_t *lock;
Copy link

Choose a reason for hiding this comment

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

Is spinlock the right concurrency primitive here?
So this is a lock per lua state, and there is a lua state per CPU, and spinlocks are only appropriate for locking accesses between different CPUs to the same data. And also, even in that context, spinlocks are highly inefficient when they are taken for a long time, such as would be the case for executing a lua script.

So if you ever happened upon a case where the same CPU got this lock multiple times, it would have hung forever, so I suspect that you might not even need a lock at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately a lock is indeed needed. If a user attempts to load a Lua script while another script is being executed in the same Lua state, we will have concurrent accesses to the a Lua State, which will cause synchronization issues. If not a spinlock, what would you suggest?

Copy link

Choose a reason for hiding this comment

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

Based on your other message, I see now that what you probably need is just local_bh_disable and local_bh_enable when loading script.

};
Copy link

Choose a reason for hiding this comment

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

So this struct xdplua is creating another unnecessary level of indirection.
so you already have this:

static DEFINE_PER_CPU(spinlock_t, lua_state_lock);

And you can easily grab this, as you already do:

xdplua->lock = per_cpu_ptr(&lua_state_lock, i);

This might be moot if you just remove the lock anyway.

Also, should be possible, and it would be much superior, if instead of having a lua_State pointer as the per cpu attribute, you just had the value directly, that would be one less unnecessary level of indirection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, maybe it would be possible not to use this struct. I'll try this out.


DECLARE_PER_CPU(struct xdplua, xdplua_per_cpu);
#endif /* CONFIG_XDP_LUA */

struct xdp_buff {
void *data;
void *data_end;
void *data_meta;
void *data_hard_start;
unsigned long handle;
struct xdp_rxq_info *rxq;
#ifdef CONFIG_XDPLUA
#ifdef CONFIG_XDP_LUA
struct sk_buff *skb;
struct lua_State *L;
#endif /* CONFIG_XDPLUA */
struct xdplua *xdplua;
#endif /* CONFIG_XDP_LUA */
};

struct xdp_frame {
Expand Down
10 changes: 6 additions & 4 deletions include/uapi/linux/bpf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2825,7 +2825,7 @@ union bpf_attr {
FN(sk_storage_get), \
FN(sk_storage_delete), \
FN(send_signal), \
/* #ifdef CONFIG_XDPLUA */ \
/* #ifdef CONFIG_XDP_LUA */ \
FN(lua_dataref), \
FN(lua_dataunref), \
FN(lua_pcall), \
Expand All @@ -2836,10 +2836,12 @@ union bpf_attr {
FN(lua_pushmap), \
FN(lua_pushskb), \
FN(lua_pushstring), \
FN(lua_setstate), \
FN(lua_toboolean), \
FN(lua_tointeger),
/* #endif CONFIG_XDPLUA */
FN(lua_tointeger), \
FN(lua_putstate), \
FN(lua_removestate), \
FN(lua_newpacket),
/* #endif CONFIG_XDP_LUA */

/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
Expand Down
4 changes: 2 additions & 2 deletions include/uapi/linux/if_link.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -974,9 +974,9 @@ enum {
IFLA_XDP_DRV_PROG_ID,
IFLA_XDP_SKB_PROG_ID,
IFLA_XDP_HW_PROG_ID,
#ifdef CONFIG_XDPLUA
#ifdef CONFIG_XDP_LUA
IFLA_XDP_LUA_PROG,
#endif /* CONFIG_XDPLUA */
#endif /* CONFIG_XDP_LUA */
__IFLA_XDP_MAX,
};

Expand Down
6 changes: 6 additions & 0 deletions init/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1288,6 +1288,12 @@ config HAVE_PCSPKR_PLATFORM
config BPF
bool

config LUNATIK
bool "Lunatik"
default n
help
Support for the Lua interpreter

menuconfig EXPERT
bool "Configure standard kernel features (expert users)"
# Unhide debug options, to make the on-by-default options visible
Expand Down
3 changes: 2 additions & 1 deletion lib/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,9 +296,10 @@ obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
obj-$(CONFIG_OBJAGG) += objagg.o

subdir-ccflags-y += -I$(srctree)/lib/lunatik/lua \
-I$(srctree)/lib/luadata/ \
-I$(srctree)/lib/luadata/ -I$(srctree)/lib/luaunpack/ \
-D_KERNEL
obj-$(CONFIG_LUNATIK) += lunatik/
obj-$(CONFIG_LUADATA) += luadata/
obj-$(CONFIG_LUAXDP) += luaxdp/
obj-$(CONFIG_LUARCU) += luarcu/
obj-$(CONFIG_LUAUNPACK) += luaunpack/
1 change: 1 addition & 0 deletions lib/luaunpack
Open in desktop
Submodule luaunpack added at 0589fc
6 changes: 4 additions & 2 deletions net/core/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ obj-y := sock.o request_sock.o skbuff.o datagram.o stream.o scm.o \

obj-$(CONFIG_SYSCTL) += sysctl_net_core.o

CFLAGS_dev.o = -Ilib/lunatik/lua/ -D_KERNEL
CFLAGS_filter.o = -Ilib/lunatik/lua/ -D_KERNEL
CFLAGS_dev.o = -Ilib/lunatik/lua/ -D_KERNEL \
-Ilib/luadata/
CFLAGS_filter.o = -Ilib/lunatik/lua/ -D_KERNEL \
-Ilib/luadata/ -Ilib/luaunpack/
obj-y += dev.o ethtool.o dev_addr_lists.o dst.o netevent.o \
neighbour.o rtnetlink.o utils.o link_watch.o filter.o \
sock_diag.o dev_ioctl.o tso.o sock_reuseport.o \
Expand Down
75 changes: 35 additions & 40 deletions net/core/dev.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@
* - netif_rx() feedback
*/

#ifdef CONFIG_XDP_LUA
#include <lua.h>
#include <lauxlib.h>
#include <lualib.h>
#include <luadata.h>
#endif /* CONFIG_XDP_LUA */

#include <linux/uaccess.h>
#include <linux/bitops.h>
#include <linux/capability.h>
Expand Down Expand Up @@ -143,12 +150,6 @@
#include <linux/indirect_call_wrapper.h>
#include <net/devlink.h>

#ifdef CONFIG_XDPLUA
#include <lua.h>
#include <lauxlib.h>
#include <lualib.h>
#endif /* CONFIG_XDPLUA */

#include "net-sysfs.h"

#define MAX_GRO_SKBS 8
Expand All @@ -158,6 +159,9 @@

static DEFINE_SPINLOCK(ptype_lock);
static DEFINE_SPINLOCK(offload_lock);
#ifdef CONFIG_XDP_LUA
static DEFINE_PER_CPU(spinlock_t, lua_state_lock);
#endif
struct list_head ptype_base[PTYPE_HASH_SIZE] __read_mostly;
struct list_head ptype_all __read_mostly; /* Taps */
static struct list_head offload_base __read_mostly;
Expand All @@ -170,10 +174,6 @@ static int call_netdevice_notifiers_extack(unsigned long val,
struct netlink_ext_ack *extack);
static struct napi_struct *napi_by_id(unsigned int napi_id);

#ifdef CONFIG_XDPLUA
struct list_head lua_state_cpu_list;
#endif /* CONFIG_XDPLUA */

/*
* The @dev_base_head list is protected by @dev_base_lock and the rtnl
* semaphore.
Expand Down Expand Up @@ -4381,9 +4381,9 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,

rxqueue = netif_get_rxqueue(skb);
xdp->rxq = &rxqueue->xdp_rxq;
#ifdef CONFIG_XDPLUA
#ifdef CONFIG_XDP_LUA
xdp->skb = skb;
#endif /* CONFIG_XDPLUA */
#endif /* CONFIG_XDP_LUA */

act = bpf_prog_run_xdp(xdp_prog, xdp);

Expand Down Expand Up @@ -5196,21 +5196,29 @@ static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp)
return ret;
}

#ifdef CONFIG_XDPLUA
#ifdef CONFIG_XDP_LUA
DEFINE_PER_CPU(struct xdplua, xdplua_per_cpu);

int generic_xdp_lua_install_prog(char *lua_prog)
{
struct lua_state_cpu *sc;
struct xdplua *sc;
int i;

list_for_each_entry(sc, &lua_state_cpu_list, list) {
for_each_possible_cpu(i) {
sc = per_cpu_ptr(&xdplua_per_cpu, i);
spin_lock_bh(sc->lock);
if (luaL_dostring(sc->L, lua_prog)) {
pr_err(KERN_INFO "error: %s\nOn cpu: %d\n",
lua_tostring(sc->L, -1), sc->cpu);
lua_tostring(sc->L, -1), i);
spin_unlock_bh(sc->lock);
return -EINVAL;
}

spin_unlock_bh(sc->lock);
}
return 0;
}
#endif /* CONFIG_XDPLUA */
#endif /* CONFIG_XDP_LUA */

static int netif_receive_skb_internal(struct sk_buff *skb)
{
Expand Down Expand Up @@ -9829,9 +9837,6 @@ static struct pernet_operations __net_initdata default_device_ops = {
static int __init net_dev_init(void)
{
int i, rc = -ENOMEM;
#ifdef CONFIG_XDPLUA
struct lua_state_cpu *new_state_cpu;
#endif /* CONFIG_XDPLUA */

BUG_ON(!dev_boot_phase);

Expand All @@ -9846,9 +9851,6 @@ static int __init net_dev_init(void)
INIT_LIST_HEAD(&ptype_base[i]);

INIT_LIST_HEAD(&offload_base);
#ifdef CONFIG_XDPLUA
INIT_LIST_HEAD(&lua_state_cpu_list);
#endif /* CONFIG_XDPLUA */

if (register_pernet_subsys(&netdev_net_ops))
goto out;
Expand All @@ -9860,6 +9862,7 @@ static int __init net_dev_init(void)
for_each_possible_cpu(i) {
struct work_struct *flush = per_cpu_ptr(&flush_works, i);
struct softnet_data *sd = &per_cpu(softnet_data, i);
struct xdplua *xdplua = per_cpu_ptr(&xdplua_per_cpu, i);

INIT_WORK(flush, flush_backlog);

Expand All @@ -9879,26 +9882,18 @@ static int __init net_dev_init(void)
init_gro_hash(&sd->backlog);
sd->backlog.poll = process_backlog;
sd->backlog.weight = weight_p;

#ifdef CONFIG_XDPLUA
new_state_cpu = (struct lua_state_cpu *)
kmalloc(sizeof(lua_state_cpu), GFP_ATOMIC);
if (!new_state_cpu)
continue;

new_state_cpu->L = luaL_newstate();
if (!new_state_cpu->L) {
kfree(new_state_cpu);
#ifdef CONFIG_XDP_LUA
xdplua->L = luaL_newstate();
if (!xdplua->L) {
kfree(xdplua);
Copy link

Choose a reason for hiding this comment

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

Leaving xdplua dangling.
Also would be weird being in a state where possibly some CPUs managed to get a lua_State, others did not.
Maybe it would make more sense to BUG_ON here.

continue;
}

luaL_openlibs(new_state_cpu->L);
luaL_requiref(new_state_cpu->L, "data", luaopen_data, 1);
lua_pop(new_state_cpu->L, 1);
new_state_cpu->cpu = i;

list_add(&new_state_cpu->list, &lua_state_cpu_list);
#endif /* CONFIG_XDPLUA */
luaL_openlibs(xdplua->L);
luaL_requiref(xdplua->L, "data", luaopen_data, 1);
lua_pop(xdplua->L, 1);
xdplua->lock = per_cpu_ptr(&lua_state_lock, i);
#endif /* CONFIG_XDP_LUA */
}

dev_boot_phase = 0;
Expand Down
Loading