| Message ID | 20150505202959.8715.51882.stgit@ivy |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 5/5/15 1:30 PM, Jesper Dangaard Brouer wrote: > > Introduce xmit_mode 'rx_inject' for pktgen which generates the packets > using familiar pktgen commands, but feeds them into > netif_receive_skb() instead of ndo_start_xmit(). ... > pgset "xmit_mode rx_inject" I think 'xmit_mode rx_inject' would make native english speaker cringe, since it's saying 'transmit mode is receive' ... but I don't mind :) > > Signed-off-by: Alexei Starovoitov <ast@plumgrid.com> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> ... > @@ -251,13 +255,14 @@ struct pktgen_dev { > * we will do a random selection from within the range. > */ > __u32 flags; > - int removal_mark; /* non-zero => the device is marked for > - * removal by worker thread */ > - > + int xmit_mode; > int min_pkt_size; > int max_pkt_size; > int pkt_overhead; /* overhead for MPLS, VLANs, IPSEC etc */ > int nfrags; > + int removal_mark; /* non-zero => the device is marked for > + * removal by worker thread */ I'm not sure why you're moving removal_mark field. Looks good. Thank you for doing this. Ack. My SOB is already there :) btw, these patches didn't reach my subscribed to netdev email yet... something is stalling vger. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 05 May 2015 21:33:26 -0700 Alexei Starovoitov <ast@plumgrid.com> wrote: > On 5/5/15 1:30 PM, Jesper Dangaard Brouer wrote: > > > > Introduce xmit_mode 'rx_inject' for pktgen which generates the packets > > using familiar pktgen commands, but feeds them into > > netif_receive_skb() instead of ndo_start_xmit(). > ... > > pgset "xmit_mode rx_inject" > > I think 'xmit_mode rx_inject' would make native english speaker cringe, > since it's saying 'transmit mode is receive' ... but I don't mind :) Yes, I know. Like Daniel suggested, I considered only calling it "rx" but it made me cringe for this exact reason, thus I extended it with "inject". I'm flexible with the name of this... > > Signed-off-by: Alexei Starovoitov <ast@plumgrid.com> > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > ... > > @@ -251,13 +255,14 @@ struct pktgen_dev { > > * we will do a random selection from within the range. > > */ > > __u32 flags; > > - int removal_mark; /* non-zero => the device is marked for > > - * removal by worker thread */ > > - > > + int xmit_mode; > > int min_pkt_size; > > int max_pkt_size; > > int pkt_overhead; /* overhead for MPLS, VLANs, IPSEC etc */ > > int nfrags; > > + int removal_mark; /* non-zero => the device is marked for > > + * removal by worker thread */ > > I'm not sure why you're moving removal_mark field. Because I wanted to place 'xmit_more' on a read-only/mostly cache-line, although it likely does not matter too much, I just wanted to avoid any funny cache coherency protocol interactions. > Looks good. Thank you for doing this. > Ack. My SOB is already there :) > > btw, these patches didn't reach my subscribed to netdev email yet... > something is stalling vger. Hmm, that is strange. I think I see them. And they are on patchwork too. https://patchwork.ozlabs.org/patch/468378/ https://patchwork.ozlabs.org/patch/468390/
On 05/05/2015 01:30 PM, Jesper Dangaard Brouer wrote: <snip> > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index 43bb215..85195b2 100644 > --- a/net/core/pktgen.c > +++ b/net/core/pktgen.c > @@ -210,6 +210,10 @@ <snip> > @@ -3320,6 +3358,7 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev) > unsigned int burst = ACCESS_ONCE(pkt_dev->burst); > struct net_device *odev = pkt_dev->odev; > struct netdev_queue *txq; > + struct sk_buff *skb; > int ret; > > /* If device is offline, then don't send */ > @@ -3352,11 +3391,45 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev) > pkt_dev->last_pkt_size = pkt_dev->skb->len; > pkt_dev->allocated_skbs++; > pkt_dev->clone_count = 0; /* reset counter */ > + if (pkt_dev->xmit_mode == M_RX_INJECT) > + pkt_dev->skb->protocol = eth_type_trans(pkt_dev->skb, > + pkt_dev->skb->dev); > } > I was just wondering. Since M_RX_INJECT is not compatible with clone_skb couldn't the lines added above be moved down into the block below so that you could avoid the additional conditional jump? > if (pkt_dev->delay && pkt_dev->last_ok) > spin(pkt_dev, pkt_dev->next_tx); > > + if (pkt_dev->xmit_mode == M_RX_INJECT) { > + skb = pkt_dev->skb; > + atomic_add(burst, &skb->users); > + local_bh_disable(); > + do { > + ret = netif_receive_skb(skb); > + if (ret == NET_RX_DROP) > + pkt_dev->errors++; > + pkt_dev->sofar++; > + pkt_dev->seq_num++; > + if (atomic_read(&skb->users) != burst) { > + /* skb was queued by rps/rfs or taps, > + * so cannot reuse this skb > + */ > + atomic_sub(burst - 1, &skb->users); > + /* get out of the loop and wait > + * until skb is consumed > + */ > + pkt_dev->last_ok = 1; > + break; > + } > + /* skb was 'freed' by stack, so clean few > + * bits and reuse it > + */ > +#ifdef CONFIG_NET_CLS_ACT > + skb->tc_verd = 0; /* reset reclass/redir ttl */ > +#endif > + } while (--burst > 0); > + goto out; /* Skips xmit_mode M_TX */ > + } > + > txq = skb_get_tx_queue(odev, pkt_dev->skb); > > local_bh_disable(); > @@ -3404,6 +3477,7 @@ xmit_more: > unlock: > HARD_TX_UNLOCK(odev, txq); > > +out: > local_bh_enable(); > > /* If pkt_dev->count is zero, then run forever */ > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 05 May 2015 22:32:34 -0700 Alexander Duyck <alexander.duyck@gmail.com> wrote: > On 05/05/2015 01:30 PM, Jesper Dangaard Brouer wrote: > > <snip> > > > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > > index 43bb215..85195b2 100644 > > --- a/net/core/pktgen.c > > +++ b/net/core/pktgen.c > > @@ -210,6 +210,10 @@ > > <snip> > > > @@ -3320,6 +3358,7 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev) > > unsigned int burst = ACCESS_ONCE(pkt_dev->burst); > > struct net_device *odev = pkt_dev->odev; > > struct netdev_queue *txq; > > + struct sk_buff *skb; > > int ret; > > > > /* If device is offline, then don't send */ > > @@ -3352,11 +3391,45 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev) > > pkt_dev->last_pkt_size = pkt_dev->skb->len; > > pkt_dev->allocated_skbs++; > > pkt_dev->clone_count = 0; /* reset counter */ > > + if (pkt_dev->xmit_mode == M_RX_INJECT) > > + pkt_dev->skb->protocol = eth_type_trans(pkt_dev->skb, > > + pkt_dev->skb->dev); > > } > > > > I was just wondering. Since M_RX_INJECT is not compatible with > clone_skb couldn't the lines added above be moved down into the block > below so that you could avoid the additional conditional jump? Sure, that is possible. I'll let Alexei answer, as it is his code. (And repost if he also thinks so...) > > if (pkt_dev->delay && pkt_dev->last_ok) > > spin(pkt_dev, pkt_dev->next_tx); > > > > + if (pkt_dev->xmit_mode == M_RX_INJECT) { > > + skb = pkt_dev->skb; > > + atomic_add(burst, &skb->users); > > + local_bh_disable(); > > + do { > > + ret = netif_receive_skb(skb); > > + if (ret == NET_RX_DROP) > > + pkt_dev->errors++; > > + pkt_dev->sofar++; > > + pkt_dev->seq_num++; > > + if (atomic_read(&skb->users) != burst) { > > + /* skb was queued by rps/rfs or taps, > > + * so cannot reuse this skb > > + */ > > + atomic_sub(burst - 1, &skb->users); > > + /* get out of the loop and wait > > + * until skb is consumed > > + */ > > + pkt_dev->last_ok = 1; > > + break; > > + } > > + /* skb was 'freed' by stack, so clean few > > + * bits and reuse it > > + */ > > +#ifdef CONFIG_NET_CLS_ACT > > + skb->tc_verd = 0; /* reset reclass/redir ttl */ > > +#endif > > + } while (--burst > 0); > > + goto out; /* Skips xmit_mode M_TX */ > > + } > > + > > txq = skb_get_tx_queue(odev, pkt_dev->skb); > > > > local_bh_disable(); > > @@ -3404,6 +3477,7 @@ xmit_more: > > unlock: > > HARD_TX_UNLOCK(odev, txq); > > > > +out: > > local_bh_enable(); > > > > /* If pkt_dev->count is zero, then run forever */ > > > > -- > > To unsubscribe from this list: send the line "unsubscribe netdev" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
On 05/06/2015 07:24 AM, Jesper Dangaard Brouer wrote: > On Tue, 05 May 2015 21:33:26 -0700 > Alexei Starovoitov <ast@plumgrid.com> wrote: >> On 5/5/15 1:30 PM, Jesper Dangaard Brouer wrote: >>> >>> Introduce xmit_mode 'rx_inject' for pktgen which generates the packets >>> using familiar pktgen commands, but feeds them into >>> netif_receive_skb() instead of ndo_start_xmit(). >> ... >>> pgset "xmit_mode rx_inject" >> >> I think 'xmit_mode rx_inject' would make native english speaker cringe, >> since it's saying 'transmit mode is receive' ... but I don't mind :) > > Yes, I know. Like Daniel suggested, I considered only calling it "rx" > but it made me cringe for this exact reason, thus I extended it with > "inject". I'm flexible with the name of this... I don't mind how you name it eventually. ;) 'xmit_mode' I think is good, and rx|tx would be symmetric. I believe you don't like "rx" due to these two out-of-tree pktgen projects you mentioned having rx capabilities. Is that correct? From my perspective, it would be more worth however to improve packet sockets and eBPF that could already do the same thing instead of a dedicated possible pktgen receive/ capturing device for such analysis. Anyway, I can also live with a rx_inject. Cheers, Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 06 May 2015 12:17:02 +0200 Daniel Borkmann <daniel@iogearbox.net> wrote: > On 05/06/2015 07:24 AM, Jesper Dangaard Brouer wrote: > > On Tue, 05 May 2015 21:33:26 -0700 > > Alexei Starovoitov <ast@plumgrid.com> wrote: > >> On 5/5/15 1:30 PM, Jesper Dangaard Brouer wrote: > >>> > >>> Introduce xmit_mode 'rx_inject' for pktgen which generates the packets > >>> using familiar pktgen commands, but feeds them into > >>> netif_receive_skb() instead of ndo_start_xmit(). > >> ... > >>> pgset "xmit_mode rx_inject" > >> > >> I think 'xmit_mode rx_inject' would make native english speaker cringe, > >> since it's saying 'transmit mode is receive' ... but I don't mind :) > > > > Yes, I know. Like Daniel suggested, I considered only calling it "rx" > > but it made me cringe for this exact reason, thus I extended it with > > "inject". I'm flexible with the name of this... > > I don't mind how you name it eventually. ;) 'xmit_mode' I think is > good, and rx|tx would be symmetric. I believe you don't like "rx" due > to these two out-of-tree pktgen projects you mentioned having rx > capabilities. Is that correct? Not correct, I really don't care about the two out-of-tree pktgen projects. My main concern is to avoid polluting the pktgen "user-interface" (more than it already is), with a bare option like "rx" which is in no-way descriptive of its functionality. > From my perspective, it would be more > worth however to improve packet sockets and eBPF that could already > do the same thing instead of a dedicated possible pktgen receive/ > capturing device for such analysis. Anyway, I can also live with a > rx_inject. We could call it "stack_inject" instead? ... to take out the confusing "rx" part of an "transmit/xmit" mode that "receives". If someone have time, I would like to see better tools that allow us to measure different parts of the stack. For now, this is what we got. And this feature provide an easy and quick way to measure the ingress code-path, which have gotten a lot of discussion lately... which after this patch can easily be resolved by measuring instead of hand-waving ;-)
On 5/6/15 1:44 AM, Jesper Dangaard Brouer wrote: >> >> I was just wondering. Since M_RX_INJECT is not compatible with >> clone_skb couldn't the lines added above be moved down into the block >> below so that you could avoid the additional conditional jump? > > Sure, that is possible. I'll let Alexei answer, as it is his code. > (And repost if he also thinks so...) I think this optimization makes sense. Cleans up the code a little too. > We could call it "stack_inject" instead? ... to take out the confusing > "rx" part of an "transmit/xmit" mode that "receives" I would vote for: 'mode netif_receive' + 'mode start_xmit' then if out-of-tree guys would want to upstream their stuff something like 'mode parse_and_consume' can fit. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
The following series introduce some pktgen changes
Patch01:
Cleanup my own work when I introduced NO_TIMESTAMP.
Patch02:
Took over patch from Alexei, and addressed my own concerns, as Alexie
is too busy with other work, and this will provide an easy tool for
measuring ingress path performance, which is a hot topic ATM.
Changes were primarily user interface related. Introduced a separate
"xmit_mode" setting, instead of stealing one of the dev flags like
Alexei did.
---
Alexei Starovoitov (1):
pktgen: introduce xmit_mode '<start_xmit|netif_receive>'
Jesper Dangaard Brouer (1):
pktgen: adjust flag NO_TIMESTAMP to be more pktgen compliant
Documentation/networking/pktgen.txt | 9 ++++
net/core/pktgen.c | 85 +++++++++++++++++++++++++++++++++--
2 files changed, 89 insertions(+), 5 deletions(-)
From: Jesper Dangaard Brouer <brouer@redhat.com> Date: Thu, 07 May 2015 16:34:29 +0200 > The following series introduce some pktgen changes > > Patch01: > Cleanup my own work when I introduced NO_TIMESTAMP. > > Patch02: > Took over patch from Alexei, and addressed my own concerns, as Alexie > is too busy with other work, and this will provide an easy tool for > measuring ingress path performance, which is a hot topic ATM. > > Changes were primarily user interface related. Introduced a separate > "xmit_mode" setting, instead of stealing one of the dev flags like > Alexei did. Series applied, thanks Jesper. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/networking/pktgen.txt b/Documentation/networking/pktgen.txt index 6199ee6..bfb5f2c 100644 --- a/Documentation/networking/pktgen.txt +++ b/Documentation/networking/pktgen.txt @@ -193,6 +193,10 @@ Examples: pgset "rate 300M" set rate to 300 Mb/s pgset "ratep 1000000" set rate to 1Mpps + pgset "xmit_mode rx_inject" RX inject packets into stack netif_receive_skb() + Works with "burst" but not with "clone_skb". + Default xmit_mode is "tx". + Sample scripts ============== @@ -310,6 +314,9 @@ flowlen rate ratep +xmit_mode + + References: ftp://robur.slu.se/pub/Linux/net-development/pktgen-testing/ ftp://robur.slu.se/pub/Linux/net-development/pktgen-testing/examples/ diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 43bb215..85195b2 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -210,6 +210,10 @@ #define T_REMDEVALL (1<<2) /* Remove all devs */ #define T_REMDEV (1<<3) /* Remove one dev */ +/* Xmit modes */ +#define M_TX 0 /* Default normal TX */ +#define M_RX_INJECT 1 /* RX inject packet into stack */ + /* If lock -- protects updating of if_list */ #define if_lock(t) spin_lock(&(t->if_lock)); #define if_unlock(t) spin_unlock(&(t->if_lock)); @@ -251,13 +255,14 @@ struct pktgen_dev { * we will do a random selection from within the range. */ __u32 flags; - int removal_mark; /* non-zero => the device is marked for - * removal by worker thread */ - + int xmit_mode; int min_pkt_size; int max_pkt_size; int pkt_overhead; /* overhead for MPLS, VLANs, IPSEC etc */ int nfrags; + int removal_mark; /* non-zero => the device is marked for + * removal by worker thread */ + struct page *page; u64 delay; /* nano-seconds */ @@ -620,6 +625,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v) if (pkt_dev->node >= 0) seq_printf(seq, " node: %d\n", pkt_dev->node); + if (pkt_dev->xmit_mode == M_RX_INJECT) + seq_puts(seq, " xmit_mode: rx_inject\n"); + seq_puts(seq, " Flags: "); if (pkt_dev->flags & F_IPV6) @@ -1081,7 +1089,8 @@ static ssize_t pktgen_if_write(struct file *file, if (len < 0) return len; if ((value > 0) && - (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING))) + ((pkt_dev->xmit_mode == M_RX_INJECT) || + !(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING))) return -ENOTSUPP; i += len; pkt_dev->clone_skb = value; @@ -1134,7 +1143,7 @@ static ssize_t pktgen_if_write(struct file *file, return len; i += len; - if ((value > 1) && + if ((value > 1) && (pkt_dev->xmit_mode == M_TX) && (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING))) return -ENOTSUPP; pkt_dev->burst = value < 1 ? 1 : value; @@ -1160,6 +1169,35 @@ static ssize_t pktgen_if_write(struct file *file, sprintf(pg_result, "ERROR: node not possible"); return count; } + if (!strcmp(name, "xmit_mode")) { + char f[32]; + + memset(f, 0, 32); + len = strn_len(&user_buffer[i], sizeof(f) - 1); + if (len < 0) + return len; + + if (copy_from_user(f, &user_buffer[i], len)) + return -EFAULT; + i += len; + + if (strcmp(f, "tx") == 0) { + pkt_dev->xmit_mode = M_TX; + } else if (strcmp(f, "rx_inject") == 0) { + /* clone_skb set earlier, not supported in this mode */ + if (pkt_dev->clone_skb > 0) + return -ENOTSUPP; + + pkt_dev->xmit_mode = M_RX_INJECT; + } else { + sprintf(pg_result, + "xmit_mode -:%s:- unknown\nAvailable modes: %s", + f, "tx, rx_inject\n"); + return count; + } + sprintf(pg_result, "OK: xmit_mode=%s", f); + return count; + } if (!strcmp(name, "flag")) { char f[32]; memset(f, 0, 32); @@ -3320,6 +3358,7 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev) unsigned int burst = ACCESS_ONCE(pkt_dev->burst); struct net_device *odev = pkt_dev->odev; struct netdev_queue *txq; + struct sk_buff *skb; int ret; /* If device is offline, then don't send */ @@ -3352,11 +3391,45 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev) pkt_dev->last_pkt_size = pkt_dev->skb->len; pkt_dev->allocated_skbs++; pkt_dev->clone_count = 0; /* reset counter */ + if (pkt_dev->xmit_mode == M_RX_INJECT) + pkt_dev->skb->protocol = eth_type_trans(pkt_dev->skb, + pkt_dev->skb->dev); } if (pkt_dev->delay && pkt_dev->last_ok) spin(pkt_dev, pkt_dev->next_tx); + if (pkt_dev->xmit_mode == M_RX_INJECT) { + skb = pkt_dev->skb; + atomic_add(burst, &skb->users); + local_bh_disable(); + do { + ret = netif_receive_skb(skb); + if (ret == NET_RX_DROP) + pkt_dev->errors++; + pkt_dev->sofar++; + pkt_dev->seq_num++; + if (atomic_read(&skb->users) != burst) { + /* skb was queued by rps/rfs or taps, + * so cannot reuse this skb + */ + atomic_sub(burst - 1, &skb->users); + /* get out of the loop and wait + * until skb is consumed + */ + pkt_dev->last_ok = 1; + break; + } + /* skb was 'freed' by stack, so clean few + * bits and reuse it + */ +#ifdef CONFIG_NET_CLS_ACT + skb->tc_verd = 0; /* reset reclass/redir ttl */ +#endif + } while (--burst > 0); + goto out; /* Skips xmit_mode M_TX */ + } + txq = skb_get_tx_queue(odev, pkt_dev->skb); local_bh_disable(); @@ -3404,6 +3477,7 @@ xmit_more: unlock: HARD_TX_UNLOCK(odev, txq); +out: local_bh_enable(); /* If pkt_dev->count is zero, then run forever */