| Message ID | 1492564489-28854-1-git-send-email-jhs@emojatatu.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2017-04-18 at 21:14 -0400, Jamal Hadi Salim wrote: > From: Jamal Hadi Salim <jhs@mojatatu.com> So there is no more limit ? How user is supposed to size the buffer for recvmsg() ? > t->tca__pad1 = 0; > @@ -1113,6 +1133,9 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb) > if (ret > 0) { > nla_nest_end(skb, nest); > ret = skb->len; > + if (nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1])) > + goto out_module_put; > + cb->args[1] = 0; I really do not see how you manage to get room to add one additional attribute, if dump had to stop at N actions, filling whole skb. You might be lucky because nla_put_u32() wants a bit less space than an action ? Presumably you need to reserve space before the dump of actions.
On 17-04-18 09:49 PM, Eric Dumazet wrote: > On Tue, 2017-04-18 at 21:14 -0400, Jamal Hadi Salim wrote: >> From: Jamal Hadi Salim <jhs@mojatatu.com> > > So there is no more limit ? How user is supposed to size the buffer for > recvmsg() ? > That part doesnt change. Ok, I believe more clarity is needed:-> Current code allows only 32 actions to be dumped at a time. This code: So lets i have 128 actions. I can fit them into 10K. I do recvmsg(..., 32K). This will make about 32K space for me to fill up the 128 actions. But the code only allows me to send 32 TCA_ACT_MAX_PRIO which takes about 2-3K. So I will do 4 runs from kernel->user to get 128 actions ;-> User space in tc is trained to expect no more than TCA_ACT_MAX_PRIO in every batch. With this change I ask the kernel to fit as many actions as possible in the 32K (all these 128 will fit in one batch). Then it has to tell user space how many are in that batch using TCAA_ACT_COUNT attribute. Make sense? cheers, jamal
On Tue, 2017-04-18 at 22:32 -0400, Jamal Hadi Salim wrote: > On 17-04-18 09:49 PM, Eric Dumazet wrote: > > On Tue, 2017-04-18 at 21:14 -0400, Jamal Hadi Salim wrote: > >> From: Jamal Hadi Salim <jhs@mojatatu.com> > > > > So there is no more limit ? How user is supposed to size the buffer for > > recvmsg() ? > > > > That part doesnt change. Ok, I believe more clarity is needed:-> > > Current code allows only 32 actions to be dumped at a time. > This code: > > So lets i have 128 actions. I can fit them into 10K. > I do recvmsg(..., 32K). This will make about 32K space for me to fill > up the 128 actions. But the code only allows me to send 32 > TCA_ACT_MAX_PRIO which takes about 2-3K. > So I will do 4 runs from kernel->user to get 128 actions ;-> > User space in tc is trained to expect no more than > TCA_ACT_MAX_PRIO in every batch. > > With this change I ask the kernel to fit as many actions as > possible in the 32K (all these 128 will fit in one batch). > Then it has to tell user space how many are in that batch > using TCAA_ACT_COUNT attribute. > > Make sense? What if we have 1024 actions, and user provides a 4KB buffer ? Normally multiple recvmsg() calls would be needed, but I do not see how the nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]) can always succeed.
On 4/18/17, 6:14 PM, Jamal Hadi Salim wrote: > From: Jamal Hadi Salim <jhs@mojatatu.com> > > When you dump hundreds of thousands of actions, getting only 32 per > dump batch even when the socket buffer and memory allocations allow > is inefficient. > > With this change, the user will get as many as possibly fitting > within the given constraints available to the kernel. > > A new top level TLV space is introduced. An attribute > TCAA_ACT_FLAGS is used to carry the flags indicating the user > is capable of processing these large dumps. Older user space which > doesnt set this flag doesnt get the large (than 32) batches. > The kernel uses the TCAA_ACT_COUNT attribute to tell the user how many > actions are put in a single batch. As such user space app knows how long > to iterate (independent of the type of action being dumped) > instead of hardcoded maximum of 32. Is the count attribute really needed ?. user space knows how to iterate multiple attributes of the same type till the end of msg. eg. fdb dumps don't use count.
On Tue, Apr 18, 2017 at 7:32 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 17-04-18 09:49 PM, Eric Dumazet wrote: >> >> On Tue, 2017-04-18 at 21:14 -0400, Jamal Hadi Salim wrote: >>> >>> From: Jamal Hadi Salim <jhs@mojatatu.com> >> >> >> So there is no more limit ? How user is supposed to size the buffer for >> recvmsg() ? >> > > That part doesnt change. Ok, I believe more clarity is needed:-> > > Current code allows only 32 actions to be dumped at a time. > This code: > > So lets i have 128 actions. I can fit them into 10K. > I do recvmsg(..., 32K). This will make about 32K space for me to fill > up the 128 actions. But the code only allows me to send 32 > TCA_ACT_MAX_PRIO which takes about 2-3K. > So I will do 4 runs from kernel->user to get 128 actions ;-> > User space in tc is trained to expect no more than > TCA_ACT_MAX_PRIO in every batch. > > With this change I ask the kernel to fit as many actions as > possible in the 32K (all these 128 will fit in one batch). > Then it has to tell user space how many are in that batch > using TCAA_ACT_COUNT attribute. > > Make sense? Hmm? How do tc filters do dumping? There is no max and no COUNT attribute either, IIUC.
On 17-04-19 12:55 AM, Roopa Prabhu wrote: > On 4/18/17, 6:14 PM, Jamal Hadi Salim wrote: >> >> A new top level TLV space is introduced. An attribute >> TCAA_ACT_FLAGS is used to carry the flags indicating the user >> is capable of processing these large dumps. Older user space which >> doesnt set this flag doesnt get the large (than 32) batches. >> The kernel uses the TCAA_ACT_COUNT attribute to tell the user how many >> actions are put in a single batch. As such user space app knows how long >> to iterate (independent of the type of action being dumped) >> instead of hardcoded maximum of 32. > Is the count attribute really needed ?. user space knows how to iterate multiple > attributes of the same type till the end of msg. eg. fdb dumps don't use count. > fdb dumping is different. One nlmsg contains one fdb entry. So the boundary for checking for iproute2 is the nlmsg header. So you wouldnt need such a counter. OTOH, actions are packed, many per nlmsg: I can put X or more policers in one nlmsg and batch many nlmsgs; all constrained by the dump skb size. Back in the day it was considered that a filter having more than 32 actions in its pipeline was probably enough. So in a dump, the kernel would not send more than 32(TCA_ACT_MAX_PRIO) at a time. This patch basically makes it fit as many as possible. The other difference is there is only one type of fdb. There are many types of actions - each with different sizes and different optionally appearing entities. So if you are writing generic code to print for example, you can play acrobatics in user space and look at TLV lengths, deduce where the boundaries are for each type and then for each type of action do speacial handling. If the kernel tells you how many actions are packed then a major change in iproute2 is: - for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { + for (i = 0; i < tot_acts_from_kernel; i++) { cheers, jamal PS: I think fdb is a good candidate for time filtering since it keeps track of lastused time. You dont really have to change it to be as efficient as actions in terms of packing but it would definetely improve status quo since you now have stats.
On 17-04-19 02:12 AM, Cong Wang wrote: > On Tue, Apr 18, 2017 at 7:32 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> On 17-04-18 09:49 PM, Eric Dumazet wrote: >> With this change I ask the kernel to fit as many actions as >> possible in the 32K (all these 128 will fit in one batch). >> Then it has to tell user space how many are in that batch >> using TCAA_ACT_COUNT attribute. >> >> Make sense? > > Hmm? How do tc filters do dumping? There is no max and no > COUNT attribute either, IIUC. > They dont have this hard coded 32 entries limit. So standard dumping works fine. Note: count is a very speacial attribute to make sure we can distinguish between old kernels which send only 32 and new kernels which could send more. cheers, jamal
On 17-04-18 11:17 PM, Eric Dumazet wrote: > On Tue, 2017-04-18 at 22:32 -0400, Jamal Hadi Salim wrote: >> On 17-04-18 09:49 PM, Eric Dumazet wrote: >>> On Tue, 2017-04-18 at 21:14 -0400, Jamal Hadi Salim wrote: >> >> Make sense? > > What if we have 1024 actions, and user provides a 4KB buffer ? > No problem - we will fit as many per batch to consume 4KB and will send them as long as user calls recvmsg. > Normally multiple recvmsg() calls would be needed, but I do not see how > the nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]) can always succeed. Oh, I see the cross-talk Eric;-> We dont pack the actions in TCAA_ACT_COUNT - we put them in TCAA_ACT_TAB. Here's some strace capture that best describes what happened before and after which i hope will make sense. Granted tc uses 32KB from user space and not 4KB you mention. We have 400 actions in the kernel at this point: tc with no changes (doesnt for this large dump): --- recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{"\240\16\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\204\16\1\0\200\0\0\0\t\0\1\0"..., 32768,g}], msg_controllen=0, msg_flags=0}, 0) = 3744 ===> total actions dumped 29 recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{" \20\0\0002\0\2\0s6\367XI\3\0\0\0\0\0\0\4\20\1\0\200\0\0\0\t\0\1\0"..., 32768}], msg_controllen=0, msg_flags=0}, 0) = 4128 ==> total actions dumped 32 recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{" \20\0\0002\0\2\0s6\367XI\3\0\0\0\0\0\0\4\20\1\0\200\0\0\0\t\0\1\0"..., 32768}], msg_controllen=0, msg_flags=0}, 0) = 4128 ==> total actions dumped 32 .... ..... ......... recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{"\24\0\0\0\3\0\2\0s6\367XI\3\0\0\0\0\0\0", 32768}], msg_controllen=0, msg_flags=0}, 0) = 20 ----- Goes on a few times until we get all 400 entries - last recvmsg (with 20B) has no actions and indicates dump is complete. Issue: The kernel is refusing to add more than 32 entries in the skb even though we get allocated 32KB for the skb. So now lets see what happens with this change: ------ recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{"\240\16\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\204\16\1\0\200\0\0\0\t\0\1\0"..., 32768,g}], msg_controllen=0, msg_flags=0}, 0) = 3744 ==> total actions dumped 29 recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{"\240~\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\204~\1\0\200\0\0\0\t\0\1\0"..., 32768,g}], msg_controllen=0, msg_flags=0}, 0) = 32416 ==> total actions dumped 253 recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{" ;\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\4;\1\0\200\0\0\0\t\0\1\0"..., 32768,g}], msg_controllen=0, msg_flags=0}, 0) = 15136 ==> total actions dumped 118 recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{"\24\0\0\0\3\0\2\0s6\367XI\3\0\0\0\0\0\0", 32768}], msg_controllen=0, msg_flags=0}, 0) = 20 -------- We got all 400 in 3 requests. Imagine what we have to deal with when we have 2M actions (the improvement is about 10x). cheers, jamal
On Wed, 2017-04-19 at 07:24 -0400, Jamal Hadi Salim wrote: > On 17-04-18 11:17 PM, Eric Dumazet wrote: > > On Tue, 2017-04-18 at 22:32 -0400, Jamal Hadi Salim wrote: > >> On 17-04-18 09:49 PM, Eric Dumazet wrote: > >>> On Tue, 2017-04-18 at 21:14 -0400, Jamal Hadi Salim wrote: > > >> > >> Make sense? > > > > What if we have 1024 actions, and user provides a 4KB buffer ? > > > > No problem - we will fit as many per batch to consume 4KB > and will send them as long as user calls recvmsg. > > > Normally multiple recvmsg() calls would be needed, but I do not see how > > the nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]) can always succeed. > > Oh, I see the cross-talk Eric;-> > We dont pack the actions in TCAA_ACT_COUNT - we put them in > TCAA_ACT_TAB. > > Here's some strace capture that best describes what happened before > and after which i hope will make sense. Granted tc uses 32KB from > user space and not 4KB you mention. > > We have 400 actions in the kernel at this point: > > tc with no changes (doesnt for this large dump): > --- > recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, > msg_iov(1)=[{"\240\16\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\204\16\1\0\200\0\0\0\t\0\1\0"..., > 32768,g}], msg_controllen=0, msg_flags=0}, 0) = 3744 > > ===> total actions dumped 29 > > recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, > msg_iov(1)=[{" > \20\0\0002\0\2\0s6\367XI\3\0\0\0\0\0\0\4\20\1\0\200\0\0\0\t\0\1\0"..., > 32768}], msg_controllen=0, msg_flags=0}, 0) = 4128 > > ==> total actions dumped 32 > > recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, > msg_iov(1)=[{" > \20\0\0002\0\2\0s6\367XI\3\0\0\0\0\0\0\4\20\1\0\200\0\0\0\t\0\1\0"..., > 32768}], msg_controllen=0, msg_flags=0}, 0) = 4128 > > ==> total actions dumped 32 > .... > ..... > ......... > recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, > msg_iov(1)=[{"\24\0\0\0\3\0\2\0s6\367XI\3\0\0\0\0\0\0", 32768}], > msg_controllen=0, msg_flags=0}, 0) = 20 > ----- > > Goes on a few times until we get all 400 entries - last recvmsg (with > 20B) has no actions and indicates dump is complete. > Issue: The kernel is refusing to add more than 32 entries in the skb > even though we get allocated 32KB for the skb. > > So now lets see what happens with this change: > ------ > recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, > msg_iov(1)=[{"\240\16\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\204\16\1\0\200\0\0\0\t\0\1\0"..., > 32768,g}], msg_controllen=0, msg_flags=0}, 0) = 3744 > > ==> total actions dumped 29 > > recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, > msg_iov(1)=[{"\240~\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\204~\1\0\200\0\0\0\t\0\1\0"..., > 32768,g}], msg_controllen=0, msg_flags=0}, 0) = 32416 > > ==> total actions dumped 253 > > recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, > msg_iov(1)=[{" > ;\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\4;\1\0\200\0\0\0\t\0\1\0"..., > 32768,g}], msg_controllen=0, msg_flags=0}, 0) = 15136 > > ==> total actions dumped 118 > > recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, > msg_iov(1)=[{"\24\0\0\0\3\0\2\0s6\367XI\3\0\0\0\0\0\0", 32768}], > msg_controllen=0, msg_flags=0}, 0) = 20 > > -------- > > We got all 400 in 3 requests. Imagine what we have to deal with > when we have 2M actions (the improvement is about 10x). Just because your strace works with the size of 32768 does not mean it will work in the future. Please read again my question. Try to _not_ use 32768 bytes for the recvmsg() sizes, but 4KB You pack XXX actions until 4KB skb is full. Then code does : nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]) This might fail, then you goto out_module_put; Then we are stuck ? What am I missing ?
On 17-04-19 09:47 AM, Eric Dumazet wrote: > Try to _not_ use 32768 bytes for the recvmsg() sizes, but 4KB > > You pack XXX actions until 4KB skb is full. > > Then code does : > > nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]) > > This might fail, then you > > goto out_module_put; > > > Then we are stuck ? > > What am I missing ? > > Ok, I understand you now ;-> You are saying we need make sure there is at least 64 bits reserved so we can stick this TLV in there. We may have to review a lot of other dumping code in general to make sure this doesnt happen. Thanks Eric. cheers, jamal
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h index cce0613..c7080ec 100644 --- a/include/uapi/linux/rtnetlink.h +++ b/include/uapi/linux/rtnetlink.h @@ -674,10 +674,27 @@ struct tcamsg { unsigned char tca__pad1; unsigned short tca__pad2; }; + +enum { + TCAA_UNSPEC, + TCAA_ACT_TAB, + TCAA_ACT_FLAGS, + TCAA_ACT_COUNT, + __TCAA_MAX +}; + +#define TCAA_MAX (__TCAA_MAX - 1) #define TA_RTA(r) ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg)))) #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg)) -#define TCA_ACT_TAB 1 /* attr type must be >=1 */ -#define TCAA_MAX 1 +#define TCA_ACT_TAB TCAA_ACT_TAB +/* tcamsg flags stored in attribute TCAA_ACT_FLAGS + * + * ACT_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO + * actions in a dump. All dump responses will contain the number of actions + * being dumped stored in for user app's consumption in TCAA_ACT_COUNT + * + */ +#define ACT_LARGE_DUMP_ON (1 << 0) /* New extended info filters for IFLA_EXT_MASK */ #define RTEXT_FILTER_VF (1 << 0) diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 82b1d48..45e1cf7 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -83,6 +83,7 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb, struct netlink_callback *cb) { int err = 0, index = -1, i = 0, s_i = 0, n_i = 0; + unsigned short act_flags = cb->args[2]; struct nlattr *nest; spin_lock_bh(&hinfo->lock); @@ -111,14 +112,18 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb, } nla_nest_end(skb, nest); n_i++; - if (n_i >= TCA_ACT_MAX_PRIO) + if (!(act_flags & ACT_LARGE_DUMP_ON) && + n_i >= TCA_ACT_MAX_PRIO) goto done; } } done: spin_unlock_bh(&hinfo->lock); - if (n_i) + if (n_i) { cb->args[0] += n_i; + if (act_flags & ACT_LARGE_DUMP_ON) + cb->args[1] = n_i; + } return n_i; nla_put_failure: @@ -993,11 +998,15 @@ static int tcf_action_add(struct net *net, struct nlattr *nla, return tcf_add_notify(net, n, &actions, portid); } +static const struct nla_policy tcaa_policy[TCAA_MAX + 1] = { + [TCAA_ACT_FLAGS] = { .type = NLA_U32 }, +}; + static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n, struct netlink_ext_ack *extack) { struct net *net = sock_net(skb->sk); - struct nlattr *tca[TCA_ACT_MAX + 1]; + struct nlattr *tca[TCAA_MAX + 1]; u32 portid = skb ? NETLINK_CB(skb).portid : 0; int ret = 0, ovr = 0; @@ -1005,7 +1014,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n, !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; - ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL, + ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, tcaa_policy, extack); if (ret < 0) return ret; @@ -1046,16 +1055,12 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n, return ret; } -static struct nlattr *find_dump_kind(const struct nlmsghdr *n) +static struct nlattr *find_dump_kind(struct nlattr **nla) { struct nlattr *tb1, *tb2[TCA_ACT_MAX + 1]; struct nlattr *tb[TCA_ACT_MAX_PRIO + 1]; - struct nlattr *nla[TCAA_MAX + 1]; struct nlattr *kind; - if (nlmsg_parse(n, sizeof(struct tcamsg), nla, TCAA_MAX, - NULL, NULL) < 0) - return NULL; tb1 = nla[TCA_ACT_TAB]; if (tb1 == NULL) return NULL; @@ -1081,9 +1086,18 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb) struct nlattr *nest; struct tc_action_ops *a_o; int ret = 0; + struct nlattr *tcaa[TCAA_MAX + 1]; struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh); - struct nlattr *kind = find_dump_kind(cb->nlh); + struct nlattr *kind = NULL; + u32 act_flags = 0; + + ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tcaa, TCAA_MAX, + tcaa_policy, NULL); + if (ret < 0) + return ret; + + kind = find_dump_kind(tcaa); if (kind == NULL) { pr_info("tc_dump_action: action bad kind\n"); return 0; @@ -1093,10 +1107,16 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb) if (a_o == NULL) return 0; + if (tcaa[TCAA_ACT_FLAGS]) + act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]); + nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, cb->nlh->nlmsg_type, sizeof(*t), 0); if (!nlh) goto out_module_put; + + cb->args[2] = act_flags; + t = nlmsg_data(nlh); t->tca_family = AF_UNSPEC; t->tca__pad1 = 0; @@ -1113,6 +1133,9 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb) if (ret > 0) { nla_nest_end(skb, nest); ret = skb->len; + if (nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1])) + goto out_module_put; + cb->args[1] = 0; } else nlmsg_trim(skb, b);