netdev - Re: [PATCH net-next 4/5] veth: introduce more xdp counters

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 22 Mar 2020 23:29:04 +0900
From:   Toshiaki Makita <toshiaki.makita1@...il.com>
To:     Lorenzo Bianconi <lorenzo@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, brouer@...hat.com,
        dsahern@...il.com, lorenzo.bianconi@...hat.com, toke@...hat.com
Subject: Re: [PATCH net-next 4/5] veth: introduce more xdp counters

On 2020/03/21 23:30, Lorenzo Bianconi wrote:
>> On 2020/03/20 22:37, Lorenzo Bianconi wrote:
>>>> On 2020/03/20 1:41, Lorenzo Bianconi wrote:
>>>>> Introduce xdp_xmit counter in order to distinguish between XDP_TX and
>>>>> ndo_xdp_xmit stats. Introduce the following ethtool counters:
>>>>> - rx_xdp_tx
>>>>> - rx_xdp_tx_errors
>>>>> - tx_xdp_xmit
>>>>> - tx_xdp_xmit_errors
>>>>> - rx_xdp_redirect
>>>>
>>>> Thank you for working on this!
>>>>
>>>>> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
>>>>> ---
>>>> ...
>>>>> @@ -395,7 +404,8 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
>>>>>     	}
>>>>>     	rcv_priv = netdev_priv(rcv);
>>>>> -	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
>>>>> +	qidx = veth_select_rxq(rcv);
>>>>> +	rq = &rcv_priv->rq[qidx];
>>>>>     	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
>>>>>     	 * side. This means an XDP program is loaded on the peer and the peer
>>>>>     	 * device is up.
>>>>> @@ -424,6 +434,17 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
>>>>>     	if (flags & XDP_XMIT_FLUSH)
>>>>>     		__veth_xdp_flush(rq);
>>>>> +	rq = &priv->rq[qidx];
>>>>
>>>> I think there is no guarantee that this rq exists. Qidx is less than
>>>> rcv->real_num_rx_queues, but not necessarily less than
>>>> dev->real_num_rx_queues.
>>>>
>>>>> +	u64_stats_update_begin(&rq->stats.syncp);
>>>>
>>>> So this can cuase NULL pointer dereference.
>>>
>>> oh right, thanks for spotting this.
>>> I think we can recompute qidx for tx netdevice in this case, doing something
>>> like:
>>>
>>> qidx = veth_select_rxq(dev);
>>> rq = &priv->rq[qidx];
>>>
>>> what do you think?
>>
>> This would not cause NULL pointer deref, but I wonder what counters you've
>> added mean.
>>
>> - rx_xdp_redirect, rx_xdp_drops, rx_xdp_tx
>>
>> These counters names will be rx_queue_[i]_rx_xdp_[redirect|drops|tx].
>> "rx_" in their names looks redundant.
> 
> yes, we can drop the "rx" prefix in the stats name for them.
> 
>> Also it looks like there is not "rx[i]_xdp_tx" counter but there is
>> "rx[i]_xdp_tx_xmit" in mlx5 from this page.
>> https://community.mellanox.com/s/article/understanding-mlx5-ethtool-counters
> 
> rx[i]_xdp_tx_xmit and rx_xdp_tx are the same, we decided to use rx_xdp_tx for
> it since it seems more clear

OK.

>> - tx_xdp_xmit, tx_xdp_xmit_errors
>>
>> These counters names will be rx_queue_[i]_tx_xdp_[xmit|xmit_errors].
>> Are these rx counters or tx counters?
> 
> tx_xdp_xmit[_errors] are used to count ndo_xmit stats so they are tx counters.
> I reused veth_stats for it just for convenience. Probably we can show them without
> rx suffix so it is clear they are transmitted by the current device.
> Another approach would be create per_cput struct to collect all tx stats.
> What do you think?

As veth_xdp_xmit really does not use tx queue but select peer rxq directly, per_cpu 
sounds more appropriate than per-queue.
One concern is consistency. Per-queue rx stats and per-cpu tx stats (or only sum of 
them?) looks inconsistent.
One alternative way is to change the queue selection login in veth_xdp_xmit and 
select txq instead of rxq. Then select peer rxq from txq, like veth_xmit. Accounting 
per queue tx stats is possible only when we can determine which txq is used.

Something like this:

static int veth_select_txq(struct net_device *dev)
{
	return smp_processor_id() % dev->real_num_tx_queues;
}

static int veth_xdp_xmit(struct net_device *dev, int n,
			 struct xdp_frame **frames, u32 flags)
{
	...
	txq = veth_select_txq(dev);
	rcv_rxq = txq; // 1-to-1 mapping from txq to peer rxq
	// Note: when XDP is enabled on rcv, this condition is always false
	if (rcv_rxq >= rcv->real_num_rx_queues)
		return -ENXIO;
	rcv_priv = netdev_priv(rcv);
	rq = &rcv_priv->rq[rcv_rxq];
	...
	// account txq stats in some way here
}

Thoughts?

Toshiaki Makita