| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150502104621.4fede885@redhat.com>
Date: Sat, 2 May 2015 10:46:21 +0200
From: Jesper Dangaard Brouer <brouer@...hat.com>
To: Alexei Starovoitov <ast@...mgrid.com>
Cc: brouer@...hat.com, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
Robert Olsson <robert@...julf.net>,
Ben Greear <greearb@...delatech.com>
Subject: Re: [PATCH v3 net-next] pktgen: introduce 'rx' mode
On Thu, 30 Apr 2015 22:12:10 -0700
Alexei Starovoitov <ast@...mgrid.com> wrote:
> Introduce 'RX' mode for pktgen which generates the packets
> using familiar pktgen commands, but feeds them into
> netif_receive_skb() instead of ndo_start_xmit().
>
> It is designed to test netif_receive_skb and ingress qdisc
> performace only. Make sure to understand how it works before
> using it for other rx benchmarking.
Hi Alexei
First of all I love the idea of modifying pktgen to performance test
the RX path.
I'm not sure the simple "rx" flag is a good "name". It likely
conflicts with other work where pktgen can receive it own packets, e.g.
https://people.kth.se/~danieltt/pktgen/ or Ben Greer's solution.
In your patch several things are not pktgen "compliant":
1. Flags in pktgen are normally in upper-case "RX"
2. Flags also require a disable "!RX" option
3. You didn't add the flag to list of supported flags
4. You don't output if the flag is enabled
5. You didn't update the documentation (Documentation/networking/pktgen.txt)
Cc.ed Robert and Ben, and left the patch below for their review.
> Signed-off-by: Alexei Starovoitov <ast@...mgrid.com>
> ---
> v2->v3: addressed more Eric comments. Thanks!
>
> v1->v2: as suggested by Eric:
> - dropped 'clone_skb' flag, now it will return enotsupp
> - fix rps/rfs bug by checking skb->users after every netif_receive_skb
> - tested with RPS/RFS, taps, veth, physical devs, various tc cls/act
>
> net/core/pktgen.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index 508155b283dd..34fd5ece2681 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -203,6 +203,7 @@
> #define F_NODE (1<<15) /* Node memory alloc*/
> #define F_UDPCSUM (1<<16) /* Include UDP checksum */
> #define F_NO_TIMESTAMP (1<<17) /* Don't timestamp packets (default TS) */
> +#define F_DO_RX (1<<18) /* generate packets for RX */
>
> /* Thread control flag bits */
> #define T_STOP (1<<0) /* Stop run */
> @@ -1081,7 +1082,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->flags & F_DO_RX) ||
> + !(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
> return -ENOTSUPP;
> i += len;
> pkt_dev->clone_skb = value;
> @@ -1089,6 +1091,12 @@ static ssize_t pktgen_if_write(struct file *file,
> sprintf(pg_result, "OK: clone_skb=%d", pkt_dev->clone_skb);
> return count;
> }
> + if (!strcmp(name, "rx")) {
> + pkt_dev->flags |= F_DO_RX;
> +
> + sprintf(pg_result, "OK: RX is ON");
> + return count;
> + }
> if (!strcmp(name, "count")) {
> len = num_arg(&user_buffer[i], 10, &value);
> if (len < 0)
> @@ -1134,7 +1142,7 @@ static ssize_t pktgen_if_write(struct file *file,
> return len;
>
> i += len;
> - if ((value > 1) &&
> + if ((value > 1) && !(pkt_dev->flags & F_DO_RX) &&
> (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
> return -ENOTSUPP;
> pkt_dev->burst = value < 1 ? 1 : value;
> @@ -3317,6 +3325,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 */
> @@ -3349,11 +3358,46 @@ 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->flags & F_DO_RX)
> + 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->flags & F_DO_RX) {
> + 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;
> + pkt_dev->clone_skb = 0;
> + 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;
> + }
> +
> txq = skb_get_tx_queue(odev, pkt_dev->skb);
>
> local_bh_disable();
> @@ -3401,6 +3445,7 @@ xmit_more:
> unlock:
> HARD_TX_UNLOCK(odev, txq);
>
> +out:
> local_bh_enable();
>
> /* If pkt_dev->count is zero, then run forever */
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists