From: Toshiaki Makita <toshiaki.makita1@gmail•com>
To: Lorenzo Bianconi <lorenzo@kernel•org>
Cc: netdev@vger•kernel.org, davem@davemloft•net, brouer@redhat•com,
dsahern@gmail•com, lorenzo.bianconi@redhat•com, toke@redhat•com
Subject: Re: [PATCH net-next 4/5] veth: introduce more xdp counters
Date: Sun, 22 Mar 2020 23:29:04 +0900 [thread overview]
Message-ID: <d8ccb8c7-0501-dc88-d2b2-ca594df885cb@gmail.com> (raw)
In-Reply-To: <20200321143013.GA3251815@lore-desk-wlan>
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@kernel•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
next prev parent reply other threads:[~2020-03-22 14:29 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-19 16:41 [PATCH net-next 0/5] add more xdp stats to veth driver Lorenzo Bianconi
2020-03-19 16:41 ` [PATCH net-next 1/5] veth: move xdp stats in a dedicated structure Lorenzo Bianconi
2020-03-19 16:41 ` [PATCH net-next 2/5] veth: introduce more specialized counters in veth_stats Lorenzo Bianconi
2020-03-19 16:41 ` [PATCH net-next 3/5] veth: distinguish between rx_drops and xdp_drops Lorenzo Bianconi
2020-03-19 16:41 ` [PATCH net-next 4/5] veth: introduce more xdp counters Lorenzo Bianconi
2020-03-20 13:22 ` Toshiaki Makita
2020-03-20 13:37 ` Lorenzo Bianconi
2020-03-21 13:38 ` Toshiaki Makita
2020-03-21 14:30 ` Lorenzo Bianconi
2020-03-22 14:29 ` Toshiaki Makita [this message]
2020-03-23 17:31 ` Lorenzo Bianconi
2020-03-24 14:21 ` Toshiaki Makita
2020-03-24 14:36 ` Lorenzo Bianconi
2020-03-25 13:08 ` Toshiaki Makita
2020-03-25 14:53 ` Lorenzo Bianconi
2020-03-19 16:41 ` [PATCH net-next 5/5] veth: remove atomic64_add from veth_xdp_xmit hotpath Lorenzo Bianconi
2020-03-20 4:25 ` [PATCH net-next 0/5] add more xdp stats to veth driver David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d8ccb8c7-0501-dc88-d2b2-ca594df885cb@gmail.com \
--to=toshiaki.makita1@gmail$(echo .)com \
--cc=brouer@redhat$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=dsahern@gmail$(echo .)com \
--cc=lorenzo.bianconi@redhat$(echo .)com \
--cc=lorenzo@kernel$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=toke@redhat$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox