On 11/03/2024 16:19, Toke Høiland-Jørgensen wrote:

Antonio Quartulli [off-list ref] writes:

quoted

Hi Toke,

On 08/03/2024 16:31, Toke Høiland-Jørgensen wrote:

quoted

Antonio Quartulli [off-list ref] writes:

quoted

+/* send skb to connected peer, if any */
+static void ovpn_queue_skb(struct ovpn_struct *ovpn, struct sk_buff *skb, struct ovpn_peer *peer)
+{
+	int ret;
+
+	if (likely(!peer))
+		/* retrieve peer serving the destination IP of this packet */
+		peer = ovpn_peer_lookup_by_dst(ovpn, skb);
+	if (unlikely(!peer)) {
+		net_dbg_ratelimited("%s: no peer to send data to\n", ovpn->dev->name);
+		goto drop;
+	}
+
+	ret = ptr_ring_produce_bh(&peer->tx_ring, skb);
+	if (unlikely(ret < 0)) {
+		net_err_ratelimited("%s: cannot queue packet to TX ring\n", peer->ovpn->dev->name);
+		goto drop;
+	}
+
+	if (!queue_work(ovpn->crypto_wq, &peer->encrypt_work))
+		ovpn_peer_put(peer);
+
+	return;
+drop:
+	if (peer)
+		ovpn_peer_put(peer);
+	kfree_skb_list(skb);
+}

So this puts packets on a per-peer 1024-packet FIFO queue with no
backpressure? That sounds like a pretty terrible bufferbloat situation.
Did you do any kind of latency-under-load testing of this, such as
running the RRUL test[0] through it?

Thanks for pointing this out.

Andrew Lunn just raised a similar point about these rings being
potential bufferbloat pitfalls.

And I totally agree.

I haven't performed any specific test, but I have already seen latency
bumping here and there under heavy load.

Andrew suggested at least reducing rings size to something like 128 and
then looking at BQL.

Do you have any hint as to what may make sense for a first
implementation, balancing complexity and good results?

Hmm, I think BQL may actually be fairly straight forward to implement
for this; if you just call netdev_tx_sent_queue() when the packet has
been encrypted and sent on to the lower layer, the BQL algorithm should
keep the ring buffer occupancy just at the level it needs to be to keep
the encryption worker busy. I am not sure if there is some weird reason
this won't work for something like this, but I can't think of any off
the top of my head. And implementing this should be fairly simple (it's
just a couple of function calls in the right places). As an example, see
this commit adding it to the mvneta driver:

a29b6235560a ("net: mvneta: add BQL support")

Not sure if some additional mechanism is needed to keep a bunch of
encrypted packets from piling up in the physical device qdisc (after
encryption), but that will be in addition, in that case.

Thank you very much - really appreciated.

I will look into the mentioned commit and will try to implement this 
logic in the next patchset iteration.

Regards,


-- 
Antonio Quartulli
OpenVPN Inc.