| Message ID | 20210809172207.3890697-2-richardsonnick@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add IMIX mode | expand |
On Mon, 9 Aug 2021 17:22:02 +0000 Nicholas Richardson wrote: > From: Nick Richardson <richardsonnick@google.com> > > Adds "imix_weights" command for specifying internet mix distribution. > > The command is in this format: > "imix_weights size_1,weight_1 size_2,weight_2 ... size_n,weight_n" > where the probability that packet size_i is picked is: > weight_i / (weight_1 + weight_2 + .. + weight_n) > > The user may provide up to 20 imix entries (size_i,weight_i) in this > command. > > The user specified imix entries will be displayed in the "Params" > section of the interface output. > > Values for clone_skb > 0 is not supported in IMIX mode. > > Summary of changes: > Add flag for enabling internet mix mode. > Add command (imix_weights) for internet mix input. > Return -ENOTSUPP when clone_skb > 0 in IMIX mode. > Display imix_weights in Params. > Create data structures to store imix entries and distribution. > > Signed-off-by: Nick Richardson <richardsonnick@google.com> > --- > net/core/pktgen.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 95 insertions(+) > > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index 7e258d255e90..83c83e1b5f28 100644 > --- a/net/core/pktgen.c > +++ b/net/core/pktgen.c > @@ -175,6 +175,8 @@ > #define IP_NAME_SZ 32 > #define MAX_MPLS_LABELS 16 /* This is the max label stack depth */ > #define MPLS_STACK_BOTTOM htonl(0x00000100) > +/* Max number of internet mix entries that can be specified in imix_weights. */ > +#define MAX_IMIX_ENTRIES 20 > > #define func_enter() pr_debug("entering %s\n", __func__); > > @@ -242,6 +244,12 @@ static char *pkt_flag_names[] = { > #define VLAN_TAG_SIZE(x) ((x)->vlan_id == 0xffff ? 0 : 4) > #define SVLAN_TAG_SIZE(x) ((x)->svlan_id == 0xffff ? 0 : 4) > > +struct imix_pkt { > + __u64 size; > + __u64 weight; > + __u64 count_so_far; no need for the __ prefix outside of uAPI. > +}; > + > struct flow_state { > __be32 cur_daddr; > int count; > @@ -343,6 +351,10 @@ struct pktgen_dev { > __u8 traffic_class; /* ditto for the (former) Traffic Class in IPv6 > (see RFC 3260, sec. 4) */ > > + /* IMIX */ > + unsigned int n_imix_entries; > + struct imix_pkt imix_entries[MAX_IMIX_ENTRIES]; > + > /* MPLS */ > unsigned int nr_labels; /* Depth of stack, 0 = no MPLS */ > __be32 labels[MAX_MPLS_LABELS]; > @@ -552,6 +564,16 @@ static int pktgen_if_show(struct seq_file *seq, void *v) > (unsigned long long)pkt_dev->count, pkt_dev->min_pkt_size, > pkt_dev->max_pkt_size); > > + if (pkt_dev->n_imix_entries > 0) { > + seq_printf(seq, " imix_weights: "); > + for (i = 0; i < pkt_dev->n_imix_entries; i++) { > + seq_printf(seq, "%llu,%llu ", > + pkt_dev->imix_entries[i].size, > + pkt_dev->imix_entries[i].weight); > + } > + seq_printf(seq, "\n"); seq_puts() > + } > + > seq_printf(seq, > " frags: %d delay: %llu clone_skb: %d ifname: %s\n", > pkt_dev->nfrags, (unsigned long long) pkt_dev->delay, > @@ -792,6 +814,61 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen) > return i; > } > > +static ssize_t get_imix_entries(const char __user *buffer, > + struct pktgen_dev *pkt_dev) > +{ > + /* Parses imix entries from user buffer. > + * The user buffer should consist of imix entries separated by spaces > + * where each entry consists of size and weight delimited by commas. > + * "size1,weight_1 size2,weight_2 ... size_n,weight_n" for example. > + */ This comments belongs before the function. > + long len; > + char c; > + int i = 0; > + const int max_digits = 10; Please order these lines longest to shortest (reverse xmas tree). > + pkt_dev->n_imix_entries = 0; > + > + do { > + unsigned long size; > + unsigned long weight; same > + > + len = num_arg(&buffer[i], max_digits, &size); > + if (len < 0) > + return len; > + i += len; > + if (get_user(c, &buffer[i])) > + return -EFAULT; > + /* Check for comma between size_i and weight_i */ > + if (c != ',') > + return -EINVAL; > + i++; > + > + if (size < 14 + 20 + 8) > + size = 14 + 20 + 8; Why overwrite instead of rejecting? > + len = num_arg(&buffer[i], max_digits, &weight); > + if (len < 0) > + return len; > + if (weight <= 0) > + return -EINVAL; > + > + pkt_dev->imix_entries[pkt_dev->n_imix_entries].size = size; > + pkt_dev->imix_entries[pkt_dev->n_imix_entries].weight = weight; > + > + i += len; > + if (get_user(c, &buffer[i])) > + return -EFAULT; What if this is the last entry? > + i++; > + pkt_dev->n_imix_entries++; > + > + if (pkt_dev->n_imix_entries > MAX_IMIX_ENTRIES) > + return -E2BIG; > + } while (c == ' '); empty line here > + return i; > +} > + > static ssize_t get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) > { > unsigned int n = 0; > @@ -960,6 +1037,18 @@ static ssize_t pktgen_if_write(struct file *file, > return count; > } > > + if (!strcmp(name, "imix_weights")) { > + if (pkt_dev->clone_skb > 0) > + return -ENOTSUPP; ENOTSUPP should not be returned to user space, please use a different one. > + len = get_imix_entries(&user_buffer[i], pkt_dev); > + if (len < 0) > + return len; > + > + i += len; > + return count; > + } > + > if (!strcmp(name, "debug")) { > len = num_arg(&user_buffer[i], 10, &value); > if (len < 0) > @@ -1082,10 +1171,16 @@ static ssize_t pktgen_if_write(struct file *file, > len = num_arg(&user_buffer[i], 10, &value); > if (len < 0) > return len; > + /* clone_skb is not supported for netif_receive xmit_mode and > + * IMIX mode. > + */ > if ((value > 0) && > ((pkt_dev->xmit_mode == M_NETIF_RECEIVE) || > !(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING))) > return -ENOTSUPP; > + if (value > 0 && pkt_dev->n_imix_entries > 0) > + return -ENOTSUPP; ditto > i += len; > pkt_dev->clone_skb = value; >
Hey Jakub, thanks for the quick response! > > + > > + len = num_arg(&buffer[i], max_digits, &size); > > + if (len < 0) > > + return len; > > + i += len; > > + if (get_user(c, &buffer[i])) > > + return -EFAULT; > > + /* Check for comma between size_i and weight_i */ > > + if (c != ',') > > + return -EINVAL; > > + i++; > > + > > + if (size < 14 + 20 + 8) > > + size = 14 + 20 + 8; > > Why overwrite instead of rejecting? I overwrite here to keep behavior similar to when pkt_size is set directly. When the pkt_size command is used the size value is overwritten to the minimum packet size (14 + 8 + 20). See the pkt_size section in pktgen_if_write(). > > > + len = num_arg(&buffer[i], max_digits, &weight); > > + if (len < 0) > > + return len; > > + if (weight <= 0) > > + return -EINVAL; > > + > > + pkt_dev->imix_entries[pkt_dev->n_imix_entries].size = size; > > + pkt_dev->imix_entries[pkt_dev->n_imix_entries].weight = weight; > > + > > + i += len; > > + if (get_user(c, &buffer[i])) > > + return -EFAULT; > > What if this is the last entry? If this is the last entry then the line terminating character is read. Similar code can be found in the get_labels() function in pktgen.c On Mon, Aug 9, 2021 at 5:05 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 9 Aug 2021 17:22:02 +0000 Nicholas Richardson wrote: > > From: Nick Richardson <richardsonnick@google.com> > > > > Adds "imix_weights" command for specifying internet mix distribution. > > > > The command is in this format: > > "imix_weights size_1,weight_1 size_2,weight_2 ... size_n,weight_n" > > where the probability that packet size_i is picked is: > > weight_i / (weight_1 + weight_2 + .. + weight_n) > > > > The user may provide up to 20 imix entries (size_i,weight_i) in this > > command. > > > > The user specified imix entries will be displayed in the "Params" > > section of the interface output. > > > > Values for clone_skb > 0 is not supported in IMIX mode. > > > > Summary of changes: > > Add flag for enabling internet mix mode. > > Add command (imix_weights) for internet mix input. > > Return -ENOTSUPP when clone_skb > 0 in IMIX mode. > > Display imix_weights in Params. > > Create data structures to store imix entries and distribution. > > > > Signed-off-by: Nick Richardson <richardsonnick@google.com> > > --- > > net/core/pktgen.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 95 insertions(+) > > > > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > > index 7e258d255e90..83c83e1b5f28 100644 > > --- a/net/core/pktgen.c > > +++ b/net/core/pktgen.c > > @@ -175,6 +175,8 @@ > > #define IP_NAME_SZ 32 > > #define MAX_MPLS_LABELS 16 /* This is the max label stack depth */ > > #define MPLS_STACK_BOTTOM htonl(0x00000100) > > +/* Max number of internet mix entries that can be specified in imix_weights. */ > > +#define MAX_IMIX_ENTRIES 20 > > > > #define func_enter() pr_debug("entering %s\n", __func__); > > > > @@ -242,6 +244,12 @@ static char *pkt_flag_names[] = { > > #define VLAN_TAG_SIZE(x) ((x)->vlan_id == 0xffff ? 0 : 4) > > #define SVLAN_TAG_SIZE(x) ((x)->svlan_id == 0xffff ? 0 : 4) > > > > +struct imix_pkt { > > + __u64 size; > > + __u64 weight; > > + __u64 count_so_far; > > no need for the __ prefix outside of uAPI. > > > +}; > > + > > struct flow_state { > > __be32 cur_daddr; > > int count; > > @@ -343,6 +351,10 @@ struct pktgen_dev { > > __u8 traffic_class; /* ditto for the (former) Traffic Class in IPv6 > > (see RFC 3260, sec. 4) */ > > > > + /* IMIX */ > > + unsigned int n_imix_entries; > > + struct imix_pkt imix_entries[MAX_IMIX_ENTRIES]; > > + > > /* MPLS */ > > unsigned int nr_labels; /* Depth of stack, 0 = no MPLS */ > > __be32 labels[MAX_MPLS_LABELS]; > > @@ -552,6 +564,16 @@ static int pktgen_if_show(struct seq_file *seq, void *v) > > (unsigned long long)pkt_dev->count, pkt_dev->min_pkt_size, > > pkt_dev->max_pkt_size); > > > > + if (pkt_dev->n_imix_entries > 0) { > > + seq_printf(seq, " imix_weights: "); > > + for (i = 0; i < pkt_dev->n_imix_entries; i++) { > > + seq_printf(seq, "%llu,%llu ", > > + pkt_dev->imix_entries[i].size, > > + pkt_dev->imix_entries[i].weight); > > + } > > + seq_printf(seq, "\n"); > > seq_puts() > > > + } > > + > > seq_printf(seq, > > " frags: %d delay: %llu clone_skb: %d ifname: %s\n", > > pkt_dev->nfrags, (unsigned long long) pkt_dev->delay, > > @@ -792,6 +814,61 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen) > > return i; > > } > > > > +static ssize_t get_imix_entries(const char __user *buffer, > > + struct pktgen_dev *pkt_dev) > > +{ > > + /* Parses imix entries from user buffer. > > + * The user buffer should consist of imix entries separated by spaces > > + * where each entry consists of size and weight delimited by commas. > > + * "size1,weight_1 size2,weight_2 ... size_n,weight_n" for example. > > + */ > > This comments belongs before the function. > > > + long len; > > + char c; > > + int i = 0; > > + const int max_digits = 10; > > Please order these lines longest to shortest (reverse xmas tree). > > > + pkt_dev->n_imix_entries = 0; > > + > > + do { > > + unsigned long size; > > + unsigned long weight; > > same > > > + > > + len = num_arg(&buffer[i], max_digits, &size); > > + if (len < 0) > > + return len; > > + i += len; > > + if (get_user(c, &buffer[i])) > > + return -EFAULT; > > + /* Check for comma between size_i and weight_i */ > > + if (c != ',') > > + return -EINVAL; > > + i++; > > + > > + if (size < 14 + 20 + 8) > > + size = 14 + 20 + 8; > > Why overwrite instead of rejecting? > > > + len = num_arg(&buffer[i], max_digits, &weight); > > + if (len < 0) > > + return len; > > + if (weight <= 0) > > + return -EINVAL; > > + > > + pkt_dev->imix_entries[pkt_dev->n_imix_entries].size = size; > > + pkt_dev->imix_entries[pkt_dev->n_imix_entries].weight = weight; > > + > > + i += len; > > + if (get_user(c, &buffer[i])) > > + return -EFAULT; > > What if this is the last entry? > > > + i++; > > + pkt_dev->n_imix_entries++; > > + > > + if (pkt_dev->n_imix_entries > MAX_IMIX_ENTRIES) > > + return -E2BIG; > > + } while (c == ' '); > > empty line here > > > + return i; > > +} > > + > > static ssize_t get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) > > { > > unsigned int n = 0; > > @@ -960,6 +1037,18 @@ static ssize_t pktgen_if_write(struct file *file, > > return count; > > } > > > > + if (!strcmp(name, "imix_weights")) { > > + if (pkt_dev->clone_skb > 0) > > + return -ENOTSUPP; > > ENOTSUPP should not be returned to user space, please use a different > one. > > > + len = get_imix_entries(&user_buffer[i], pkt_dev); > > + if (len < 0) > > + return len; > > + > > + i += len; > > + return count; > > + } > > + > > if (!strcmp(name, "debug")) { > > len = num_arg(&user_buffer[i], 10, &value); > > if (len < 0) > > @@ -1082,10 +1171,16 @@ static ssize_t pktgen_if_write(struct file *file, > > len = num_arg(&user_buffer[i], 10, &value); > > if (len < 0) > > return len; > > + /* clone_skb is not supported for netif_receive xmit_mode and > > + * IMIX mode. > > + */ > > if ((value > 0) && > > ((pkt_dev->xmit_mode == M_NETIF_RECEIVE) || > > !(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING))) > > return -ENOTSUPP; > > + if (value > 0 && pkt_dev->n_imix_entries > 0) > > + return -ENOTSUPP; > > ditto > > > i += len; > > pkt_dev->clone_skb = value; > > > -- Nick Richardson (he/him/his) SWE Intern 1 (919) 410 3510 careers.google.com/students |Learn more about our candidate privacy policy.|
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 7e258d255e90..83c83e1b5f28 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -175,6 +175,8 @@ #define IP_NAME_SZ 32 #define MAX_MPLS_LABELS 16 /* This is the max label stack depth */ #define MPLS_STACK_BOTTOM htonl(0x00000100) +/* Max number of internet mix entries that can be specified in imix_weights. */ +#define MAX_IMIX_ENTRIES 20 #define func_enter() pr_debug("entering %s\n", __func__); @@ -242,6 +244,12 @@ static char *pkt_flag_names[] = { #define VLAN_TAG_SIZE(x) ((x)->vlan_id == 0xffff ? 0 : 4) #define SVLAN_TAG_SIZE(x) ((x)->svlan_id == 0xffff ? 0 : 4) +struct imix_pkt { + __u64 size; + __u64 weight; + __u64 count_so_far; +}; + struct flow_state { __be32 cur_daddr; int count; @@ -343,6 +351,10 @@ struct pktgen_dev { __u8 traffic_class; /* ditto for the (former) Traffic Class in IPv6 (see RFC 3260, sec. 4) */ + /* IMIX */ + unsigned int n_imix_entries; + struct imix_pkt imix_entries[MAX_IMIX_ENTRIES]; + /* MPLS */ unsigned int nr_labels; /* Depth of stack, 0 = no MPLS */ __be32 labels[MAX_MPLS_LABELS]; @@ -552,6 +564,16 @@ static int pktgen_if_show(struct seq_file *seq, void *v) (unsigned long long)pkt_dev->count, pkt_dev->min_pkt_size, pkt_dev->max_pkt_size); + if (pkt_dev->n_imix_entries > 0) { + seq_printf(seq, " imix_weights: "); + for (i = 0; i < pkt_dev->n_imix_entries; i++) { + seq_printf(seq, "%llu,%llu ", + pkt_dev->imix_entries[i].size, + pkt_dev->imix_entries[i].weight); + } + seq_printf(seq, "\n"); + } + seq_printf(seq, " frags: %d delay: %llu clone_skb: %d ifname: %s\n", pkt_dev->nfrags, (unsigned long long) pkt_dev->delay, @@ -792,6 +814,61 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen) return i; } +static ssize_t get_imix_entries(const char __user *buffer, + struct pktgen_dev *pkt_dev) +{ + /* Parses imix entries from user buffer. + * The user buffer should consist of imix entries separated by spaces + * where each entry consists of size and weight delimited by commas. + * "size1,weight_1 size2,weight_2 ... size_n,weight_n" for example. + */ + long len; + char c; + int i = 0; + const int max_digits = 10; + + pkt_dev->n_imix_entries = 0; + + do { + unsigned long size; + unsigned long weight; + + len = num_arg(&buffer[i], max_digits, &size); + if (len < 0) + return len; + i += len; + if (get_user(c, &buffer[i])) + return -EFAULT; + /* Check for comma between size_i and weight_i */ + if (c != ',') + return -EINVAL; + i++; + + if (size < 14 + 20 + 8) + size = 14 + 20 + 8; + + len = num_arg(&buffer[i], max_digits, &weight); + if (len < 0) + return len; + if (weight <= 0) + return -EINVAL; + + pkt_dev->imix_entries[pkt_dev->n_imix_entries].size = size; + pkt_dev->imix_entries[pkt_dev->n_imix_entries].weight = weight; + + i += len; + if (get_user(c, &buffer[i])) + return -EFAULT; + + i++; + pkt_dev->n_imix_entries++; + + if (pkt_dev->n_imix_entries > MAX_IMIX_ENTRIES) + return -E2BIG; + } while (c == ' '); + return i; +} + static ssize_t get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) { unsigned int n = 0; @@ -960,6 +1037,18 @@ static ssize_t pktgen_if_write(struct file *file, return count; } + if (!strcmp(name, "imix_weights")) { + if (pkt_dev->clone_skb > 0) + return -ENOTSUPP; + + len = get_imix_entries(&user_buffer[i], pkt_dev); + if (len < 0) + return len; + + i += len; + return count; + } + if (!strcmp(name, "debug")) { len = num_arg(&user_buffer[i], 10, &value); if (len < 0) @@ -1082,10 +1171,16 @@ static ssize_t pktgen_if_write(struct file *file, len = num_arg(&user_buffer[i], 10, &value); if (len < 0) return len; + /* clone_skb is not supported for netif_receive xmit_mode and + * IMIX mode. + */ if ((value > 0) && ((pkt_dev->xmit_mode == M_NETIF_RECEIVE) || !(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING))) return -ENOTSUPP; + if (value > 0 && pkt_dev->n_imix_entries > 0) + return -ENOTSUPP; + i += len; pkt_dev->clone_skb = value;