[virtio-dev] Re: [RFC PATCH v3] can: virtio: Initial virtio CAN driver.
Hello Vincent, (Marc, you are now in the "To" as I have a direct question to you.) On 12.05.23 11:53, Vincent MAILHOL wrote: Hi Mikhail, Thanks for your patch. Do you have any plans for CAN error reporting aside from the bus off? (e.g. can error-active, can error-passive, bit errors...) On Fri. 12 May 2023 at 00:18, Mikhail Golubev-Ciuchea wrote: From: Harald Mommer - CAN Control - "ip link set up can0" starts the virtual CAN controller, - "ip link set up can0" stops the virtual CAN controller - CAN RX Receive CAN frames. CAN frames can be standard or extended, classic or CAN FD. Classic CAN RTR frames are supported. - CAN TX Send CAN frames. CAN frames can be standard or extended, classic or CAN FD. Classic CAN RTR frames are supported. - CAN BusOff indication CAN BusOff is handled by a bit in the configuration space. Signed-off-by: Harald Mommer Signed-off-by: Mikhail Golubev-Ciuchea Co-developed-by: Marc Kleine-Budde Signed-off-by: Marc Kleine-Budde Cc: Damir Shaikhutdinov --- V3: * Incorporate patch "[PATCH] can: virtio-can: cleanups" from https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2fall%2f20230424%2dfootwear%2ddaily%2d9339bd0ec428%2dmkl%40pengutronix.de%2f=9bd91fe2-cd6c-4f97-823b-3f938eb06afa=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-17d3c64a4691a4e8987f693a9c0d91fa09fb4a4f * Add missing can_free_echo_skb() * Replace home-brewed ID allocator with the standard one from kernel * Simplify flow control * Tested with https://github.com/OpenSynergy/qemu/tree/virtio-can-spec-rfc-v3 V2: * Remove the event indication queue and use the config space instead, to indicate a bus off condition * Rework RX and TX messages having a length field and some more fields for CAN EXT * Fix CAN_EFF_MASK comparison * Remove MISRA style code (e.g. '! = 0u') * Remove priorities leftovers * Remove BUGONs * Based on virtio can spec RFCv3 * Tested with https://github.com/OpenSynergy/qemu/tree/virtio-can-spec-rfc-v3 MAINTAINERS |7 + drivers/net/can/Kconfig | 12 + drivers/net/can/Makefile|3 +- drivers/net/can/virtio_can.c| 1000 +++ include/uapi/linux/virtio_can.h | 71 +++ 5 files changed, 1092 insertions(+), 1 deletion(-) create mode 100644 drivers/net/can/virtio_can.c create mode 100644 include/uapi/linux/virtio_can.h diff --git a/MAINTAINERS b/MAINTAINERS index 7e0b87d5aa2e..ca45950c8364 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -22246,6 +22246,13 @@ F: drivers/vhost/scsi.c F: include/uapi/linux/virtio_blk.h F: include/uapi/linux/virtio_scsi.h +VIRTIO CAN DRIVER +M: "Harald Mommer" +L: linux-...@vger.kernel.org +S: Maintained +F: drivers/net/can/virtio_can.c +F: include/uapi/linux/virtio_can.h + VIRTIO CONSOLE DRIVER M: Amit Shah L: virtualizat...@lists.linux-foundation.org diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index 3ceccafd701b..ee47fdff2252 100644 --- a/drivers/net/can/Kconfig +++ b/drivers/net/can/Kconfig @@ -210,6 +210,18 @@ config CAN_XILINXCAN Xilinx CAN driver. This driver supports both soft AXI CAN IP and Zynq CANPS IP. +config CAN_VIRTIO_CAN + depends on VIRTIO + tristate "Virtio CAN device support" + default n + help + Say Y here if you want to support for Virtio CAN. Broken grammar. Do either Say Y here if you want to add support for Virtio CAN. Add "add". or: Say Y here if you want to support Virtio CAN devices. + + To compile this driver as a module, choose M here: the + module will be called virtio-can. + + If unsure, say N. + source "drivers/net/can/c_can/Kconfig" source "drivers/net/can/cc770/Kconfig" source "drivers/net/can/ctucanfd/Kconfig" diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile index ff8f76295d13..19314adaff59 100644 --- a/drivers/net/can/Makefile +++ b/drivers/net/can/Makefile @@ -17,8 +17,8 @@ obj-$(CONFIG_CAN_AT91)+= at91_can.o obj-$(CONFIG_CAN_BXCAN)+= bxcan.o obj-$(CONFIG_CAN_CAN327) += can327.o obj-$(CONFIG_CAN_CC770)+= cc770/ -obj-$(CONFIG_CAN_C_CAN)+= c_can/ obj-$(CONFIG_CAN_CTUCANFD) += ctucanfd/ +obj-$(CONFIG_CAN_C_CAN)+= c_can/ This reordering is unrelated to this patch goal. Please send it as a separate patch. @Marc Kleine-Budde: We got this reordering change from you. How to proceed? We can split this in 2 commits, reordering and on top adding virtio CAN. No issue, a question of minutes and done. Fine. But here the word "patch" was used, not the word "commit". Sending a separate patch to somewhere? Maybe Mikhail does this fight to get this in (unlikely), I personally would prefer to run away. Or we don't reorder at all, wrong ordering remains and we will not make only
RE: [virtio-dev] Re: [PATCH v2 0/2] transport-pci: Introduce legacy registers access using AQ
> From: virtio-dev@lists.oasis-open.org On > Behalf Of Michael S. Tsirkin > Sent: Thursday, May 11, 2023 9:45 AM > > On Thu, May 11, 2023 at 01:15:15PM +, Parav Pandit wrote: > > > Why not simply invent individual commands to access legacy > > > facilities like commands to access like what transport virtqueue did > > > for modern > > > device?: > > > > > I don’t see how this is being any different than register-offset interface. > > For example legacy device config's offset moves depending on MSI enable. > Which is a missing piece in the current proposal btw as I keep saying. How is this missing? The offset is decided by the driver when msi enable. Device also know that msi is enable/disabled. So device responds based on what is done with msi, like how a device responds today on iobar. I responded in this previously, don’t have the link, not sure if you got chance to read through.
Re: [virtio-dev] [RFC PATCH 1/1] can: virtio: Initial virtio CAN driver.
Hello Vincent, searched for the old E-Mail, this was one of that which slipped through. Too much of those. On 05.11.22 10:21, Vincent Mailhol wrote: On Fry. 4 nov. 2022 at 20:13, Arnd Bergmann wrote: On Thu, Nov 3, 2022, at 13:26, Harald Mommer wrote: On 25.08.22 20:21, Arnd Bergmann wrote: ... The messages are not necessarily processed in sequence by the CAN stack. CAN is priority based. The lower the CAN ID the higher the priority. So a message with CAN ID 0x100 can surpass a message with ID 0x123 if the hardware is not just simple basic CAN controller using a single TX mailbox with a FIFO queue on top of it. Really? I acknowledge that it is priority based *on the bus*, i.e. if two devices A and B on the same bus try to send CAN ID 0x100 and 0x123 at the same time, then device A will win the CAN arbitration. However, I am not aware of any devices which reorder their own stack according to the CAN IDs. If I first send CAN ID 0x123 and then ID 0x100 on the device stack, 0x123 would still go out first, right? The CAN hardware may be a basic CAN hardware: Single mailbox only with a TX FIFO on top of this. No reordering takes place, the CAN hardware will try to arbitrate the CAN bus with a low priority CAN message (big CAN ID) while some high priority CAN message (small CAN ID) is waiting in the FIFO. This is called "internal priority inversion", a property of basic CAN hardware. A basic CAN hardware does exactly what you describe. Should be the FIFO in software it's a bad idea to try to improve this doing some software sorting, the processing time needed is likely to make things even worse. Therefore no software does this or at least it's not recommended to do this. But the hardware may also be a better one. No FIFO but a lot of TX mailboxes. A full CAN hardware tries to arbitrate the bus using the highest priority waiting CAN message considering all hardware TX mailboxes. Such a better (full CAN) hardware does not cause "internal priority inversion" but tries to arbitrate the bus in the correct order given by the message IDs. We don't know about the actually used CAN hardware and how it's used on this level we are with our virtio can device. We are using SocketCAN, no information about the properties of the underlying hardware is provided at some API. May be basic CAN using a FIFO and a single TX mailbox or full CAN using a lot of TX mailboxes in parallel. On the bus it's guaranteed always that the sender with the lowest CAN ID winds regardless which hardware is used, the only difference is whether we have "internal priority inversion" or not. If I look at the CAN stack = Software + hardware (and not only software) it's correct: The hardware device may re-order if it's a better (full CAN) one and thus the actual sending on the bus is not done in the same sequence as the messages were provided internally (e.g. at some socket). Regards Harald - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [RFC PATCH v3] can: virtio: Initial virtio CAN driver.
On 11.05.23 17:40, Simon Horman wrote:
On Thu, May 11, 2023 at 05:14:44PM +0200, Mikhail Golubev-Ciuchea wrote:
From: Harald Mommer
- CAN Control
- "ip link set up can0" starts the virtual CAN controller,
- "ip link set up can0" stops the virtual CAN controller
- CAN RX
Receive CAN frames. CAN frames can be standard or extended, classic or
CAN FD. Classic CAN RTR frames are supported.
- CAN TX
Send CAN frames. CAN frames can be standard or extended, classic or
CAN FD. Classic CAN RTR frames are supported.
- CAN BusOff indication
CAN BusOff is handled by a bit in the configuration space.
Signed-off-by: Harald Mommer
Signed-off-by: Mikhail Golubev-Ciuchea
Co-developed-by: Marc Kleine-Budde
Signed-off-by: Marc Kleine-Budde
Cc: Damir Shaikhutdinov
Hi Mikhail,
thanks for your patch.
Some minor feedback from my side.
...
diff --git a/drivers/net/can/virtio_can.c b/drivers/net/can/virtio_can.c
...
+/* Send a control message with message type either
+ *
+ * - VIRTIO_CAN_SET_CTRL_MODE_START or
+ * - VIRTIO_CAN_SET_CTRL_MODE_STOP.
+ *
+ * Unlike AUTOSAR CAN Driver Can_SetControllerMode() there is no requirement
+ * for this Linux driver to have an asynchronous implementation of the mode
+ * setting function so in order to keep things simple the function is
+ * implemented as synchronous function. Design pattern is
+ * virtio_console.c/__send_control_msg() & virtio_net.c/virtnet_send_command().
+ */
+static u8 virtio_can_send_ctrl_msg(struct net_device *ndev, u16 msg_type)
+{
+ struct virtio_can_priv *priv = netdev_priv(ndev);
+ struct device *dev = >vdev->dev;
+ struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_CONTROL];
+ struct scatterlist sg_out[1];
+ struct scatterlist sg_in[1];
+ struct scatterlist *sgs[2];
+ int err;
+ unsigned int len;
nit: For networking code please arrange local variables in reverse xmas
tree order - longest line to shortest.
You can check this using:https://github.com/ecree-solarflare/xmastree
In this case I think it would be:
struct virtio_can_priv *priv = netdev_priv(ndev);
struct device *dev = >vdev->dev;
struct scatterlist sg_out[1];
struct scatterlist sg_in[1];
struct scatterlist *sgs[2];
struct virtqueue *vq;
unsigned int len;
int err;
vq = priv->vqs[VIRTIO_CAN_QUEUE_CONTROL];
...
https://docs.kernel.org/process/maintainer-netdev.html
I see, I see...
+static netdev_tx_t virtio_can_start_xmit(struct sk_buff *skb,
+struct net_device *dev)
+{
+ struct virtio_can_priv *priv = netdev_priv(dev);
+ struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+ struct virtio_can_tx *can_tx_msg;
+ struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_TX];
+ struct scatterlist sg_out[1];
+ struct scatterlist sg_in[1];
+ struct scatterlist *sgs[2];
+ unsigned long flags;
+ u32 can_flags;
+ int err;
+ int putidx;
+ netdev_tx_t xmit_ret = NETDEV_TX_OK;
+ const unsigned int hdr_size = offsetof(struct virtio_can_tx_out, sdu);
+
+ if (can_dev_dropped_skb(dev, skb))
+ goto kick; /* No way to return NET_XMIT_DROP here */
+
+ /* No local check for CAN_RTR_FLAG or FD frame against negotiated
+* features. The device will reject those anyway if not supported.
+*/
+
+ can_tx_msg = kzalloc(sizeof(*can_tx_msg), GFP_ATOMIC);
+ if (!can_tx_msg)
+ goto kick; /* No way to return NET_XMIT_DROP here */
+
+ can_tx_msg->tx_out.msg_type = cpu_to_le16(VIRTIO_CAN_TX);
+ can_flags = 0;
+
+ if (cf->can_id & CAN_EFF_FLAG) {
+ can_flags |= VIRTIO_CAN_FLAGS_EXTENDED;
+ can_tx_msg->tx_out.can_id = cpu_to_le32(cf->can_id &
CAN_EFF_MASK);
+ } else {
+ can_tx_msg->tx_out.can_id = cpu_to_le32(cf->can_id &
CAN_SFF_MASK);
+ }
+ if (cf->can_id & CAN_RTR_FLAG)
+ can_flags |= VIRTIO_CAN_FLAGS_RTR;
+ else
+ memcpy(can_tx_msg->tx_out.sdu, cf->data, cf->len);
+ if (can_is_canfd_skb(skb))
+ can_flags |= VIRTIO_CAN_FLAGS_FD;
+
+ can_tx_msg->tx_out.flags = cpu_to_le32(can_flags);
+ can_tx_msg->tx_out.length = cpu_to_le16(cf->len);
+
+ /* Prepare sending of virtio message */
+ sg_init_one(_out[0], _tx_msg->tx_out, hdr_size + cf->len);
+ sg_init_one(_in[0], _tx_msg->tx_in, sizeof(can_tx_msg->tx_in));
+ sgs[0] = sg_out;
+ sgs[1] = sg_in;
+
+ putidx = virtio_can_alloc_tx_idx(priv);
+
+ if (unlikely(putidx < 0)) {
+ netif_stop_queue(dev);
+ kfree(can_tx_msg);
+ netdev_warn(dev, "TX: Stop queue, no putidx available\n");
If I understand things correctly, this code is on the datapath.
So perhaps these should be rate limited, or only logged once.
Likewise
[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH v13] virtio-net: support inner header hash
On Fri, May 12, 2023 at 03:23:46PM +0800, Heng Qi wrote: > On Fri, May 12, 2023 at 02:54:34AM -0400, Michael S. Tsirkin wrote: > > On Fri, May 12, 2023 at 02:00:19PM +0800, Heng Qi wrote: > > > On Thu, May 11, 2023 at 02:22:12AM -0400, Michael S. Tsirkin wrote: > > > > On Wed, May 10, 2023 at 05:15:37PM +0800, Heng Qi wrote: > > > > > > > > > > > > > > > 在 2023/5/9 下午11:15, Michael S. Tsirkin 写道: > > > > > > On Tue, May 09, 2023 at 10:22:19PM +0800, Heng Qi wrote: > > > > > > > > > > > > > > 在 2023/5/5 下午10:56, Michael S. Tsirkin 写道: > > > > > > > > On Fri, May 05, 2023 at 09:51:15PM +0800, Heng Qi wrote: > > > > > > > > > On Thu, Apr 27, 2023 at 01:13:29PM -0400, Michael S. Tsirkin > > > > > > > > > wrote: > > > > > > > > > > On Thu, Apr 27, 2023 at 10:28:29AM +0800, Heng Qi wrote: > > > > > > > > > > > 在 2023/4/26 下午10:48, Michael S. Tsirkin 写道: > > > > > > > > > > > > On Wed, Apr 26, 2023 at 10:14:30PM +0800, Heng Qi wrote: > > > > > > > > > > > > > This does not mean that every device needs to > > > > > > > > > > > > > implement and support all of > > > > > > > > > > > > > these, they can choose to support some protocols they > > > > > > > > > > > > > want. > > > > > > > > > > > > > > > > > > > > > > > > > > I add these because we have scale application > > > > > > > > > > > > > scenarios for modern protocols > > > > > > > > > > > > > VXLAN-GPE/GENEVE: > > > > > > > > > > > > > > > > > > > > > > > > > > +\item In scenarios where the same flow passing > > > > > > > > > > > > > through different tunnels is expected to be received > > > > > > > > > > > > > in the same queue, > > > > > > > > > > > > > + warm caches, lessing locking, etc. are > > > > > > > > > > > > > optimized to obtain receiving performance. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Maybe the legacy GRE, VXLAN-GPE and GENEVE? But it > > > > > > > > > > > > > has a little crossover. > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > But VXLAN-GPE/GENEVE can use source port for entropy. > > > > > > > > > > > > > > > > > > > > > > > > It is recommended that the UDP source port > > > > > > > > > > > > number > > > > > > > > > > > > be calculated using a hash of fields from the > > > > > > > > > > > > inner packet > > > > > > > > > > > > > > > > > > > > > > > > That is best because > > > > > > > > > > > > it allows end to end control and is protocol agnostic. > > > > > > > > > > > Yes. I agree with this, I don't think we have an argument > > > > > > > > > > > on this point > > > > > > > > > > > right now.:) > > > > > > > > > > > > > > > > > > > > > > For VXLAN-GPE/GENEVE or other modern tunneling protocols, > > > > > > > > > > > we have to deal > > > > > > > > > > > with > > > > > > > > > > > scenarios where the same flow passes through different > > > > > > > > > > > tunnels. > > > > > > > > > > > > > > > > > > > > > > Having them hashed to the same rx queue, is hard to do > > > > > > > > > > > via outer headers. > > > > > > > > > > > > All that is missing is symmetric Toepliz and all is > > > > > > > > > > > > well? > > > > > > > > > > > The scenarios above or in the commit log also require > > > > > > > > > > > inner headers. > > > > > > > > > > Hmm I am not sure I get it 100%. > > > > > > > > > > Could you show an example with inner header hash in the > > > > > > > > > > port #, > > > > > > > > > > hash is symmetric, and you still have trouble? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It kinds of sounds like not enough entropy is not the > > > > > > > > > > problem > > > > > > > > > > at this point. > > > > > > > > > Sorry for the late reply. :) > > > > > > > > > > > > > > > > > > For modern tunneling protocols, yes. > > > > > > > > > > > > > > > > > > > You now want to drop everything from the header > > > > > > > > > > except the UDP source port. Is that a fair summary? > > > > > > > > > > > > > > > > > > > For example, for the same flow passing through different > > > > > > > > > VXLAN tunnels, > > > > > > > > > packets in this flow have the same inner header and different > > > > > > > > > outer > > > > > > > > > headers. Sometimes these packets of the flow need to be > > > > > > > > > hashed to the > > > > > > > > > same rxq, then we can use the inner header as the hash input. > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > So, they will have the same source port yes? > > > > > > > Yes. The outer source port can be calculated using the 5-tuple of > > > > > > > the > > > > > > > original packet, > > > > > > > and the outer ports are the same but the outer IPs are different > > > > > > > after > > > > > > > different directions of the same flow pass through different > > > > > > > tunnels. > > > > > > > > Any way to use that > > > > > > > We use it in monitoring, firewall and other scenarios. > > > > > > > > > > > > > > > so we don't depend on a specific protocol? > > > > >
[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH v13] virtio-net: support inner header hash
On Fri, May 12, 2023 at 02:54:34AM -0400, Michael S. Tsirkin wrote: > On Fri, May 12, 2023 at 02:00:19PM +0800, Heng Qi wrote: > > On Thu, May 11, 2023 at 02:22:12AM -0400, Michael S. Tsirkin wrote: > > > On Wed, May 10, 2023 at 05:15:37PM +0800, Heng Qi wrote: > > > > > > > > > > > > 在 2023/5/9 下午11:15, Michael S. Tsirkin 写道: > > > > > On Tue, May 09, 2023 at 10:22:19PM +0800, Heng Qi wrote: > > > > > > > > > > > > 在 2023/5/5 下午10:56, Michael S. Tsirkin 写道: > > > > > > > On Fri, May 05, 2023 at 09:51:15PM +0800, Heng Qi wrote: > > > > > > > > On Thu, Apr 27, 2023 at 01:13:29PM -0400, Michael S. Tsirkin > > > > > > > > wrote: > > > > > > > > > On Thu, Apr 27, 2023 at 10:28:29AM +0800, Heng Qi wrote: > > > > > > > > > > 在 2023/4/26 下午10:48, Michael S. Tsirkin 写道: > > > > > > > > > > > On Wed, Apr 26, 2023 at 10:14:30PM +0800, Heng Qi wrote: > > > > > > > > > > > > This does not mean that every device needs to implement > > > > > > > > > > > > and support all of > > > > > > > > > > > > these, they can choose to support some protocols they > > > > > > > > > > > > want. > > > > > > > > > > > > > > > > > > > > > > > > I add these because we have scale application scenarios > > > > > > > > > > > > for modern protocols > > > > > > > > > > > > VXLAN-GPE/GENEVE: > > > > > > > > > > > > > > > > > > > > > > > > +\item In scenarios where the same flow passing through > > > > > > > > > > > > different tunnels is expected to be received in the > > > > > > > > > > > > same queue, > > > > > > > > > > > > + warm caches, lessing locking, etc. are optimized > > > > > > > > > > > > to obtain receiving performance. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Maybe the legacy GRE, VXLAN-GPE and GENEVE? But it has > > > > > > > > > > > > a little crossover. > > > > > > > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > But VXLAN-GPE/GENEVE can use source port for entropy. > > > > > > > > > > > > > > > > > > > > > > It is recommended that the UDP source port number > > > > > > > > > > >be calculated using a hash of fields from the inner > > > > > > > > > > > packet > > > > > > > > > > > > > > > > > > > > > > That is best because > > > > > > > > > > > it allows end to end control and is protocol agnostic. > > > > > > > > > > Yes. I agree with this, I don't think we have an argument > > > > > > > > > > on this point > > > > > > > > > > right now.:) > > > > > > > > > > > > > > > > > > > > For VXLAN-GPE/GENEVE or other modern tunneling protocols, > > > > > > > > > > we have to deal > > > > > > > > > > with > > > > > > > > > > scenarios where the same flow passes through different > > > > > > > > > > tunnels. > > > > > > > > > > > > > > > > > > > > Having them hashed to the same rx queue, is hard to do via > > > > > > > > > > outer headers. > > > > > > > > > > > All that is missing is symmetric Toepliz and all is well? > > > > > > > > > > The scenarios above or in the commit log also require inner > > > > > > > > > > headers. > > > > > > > > > Hmm I am not sure I get it 100%. > > > > > > > > > Could you show an example with inner header hash in the port > > > > > > > > > #, > > > > > > > > > hash is symmetric, and you still have trouble? > > > > > > > > > > > > > > > > > > > > > > > > > > > It kinds of sounds like not enough entropy is not the problem > > > > > > > > > at this point. > > > > > > > > Sorry for the late reply. :) > > > > > > > > > > > > > > > > For modern tunneling protocols, yes. > > > > > > > > > > > > > > > > > You now want to drop everything from the header > > > > > > > > > except the UDP source port. Is that a fair summary? > > > > > > > > > > > > > > > > > For example, for the same flow passing through different VXLAN > > > > > > > > tunnels, > > > > > > > > packets in this flow have the same inner header and different > > > > > > > > outer > > > > > > > > headers. Sometimes these packets of the flow need to be hashed > > > > > > > > to the > > > > > > > > same rxq, then we can use the inner header as the hash input. > > > > > > > > > > > > > > > > Thanks! > > > > > > > So, they will have the same source port yes? > > > > > > Yes. The outer source port can be calculated using the 5-tuple of > > > > > > the > > > > > > original packet, > > > > > > and the outer ports are the same but the outer IPs are different > > > > > > after > > > > > > different directions of the same flow pass through different > > > > > > tunnels. > > > > > > > Any way to use that > > > > > > We use it in monitoring, firewall and other scenarios. > > > > > > > > > > > > > so we don't depend on a specific protocol? > > > > > > Yes, selected tunneling protocols can be used in this scenario like > > > > > > this. > > > > > > > > > > > > Thanks. > > > > > > > > > > > No, the question was - can we generalize this somehow then? > > > > > For example, a flag to ignore source IP when hashing? > > > > > Or maybe just for UDP packets? > > > > > > > > 1.
[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH v13] virtio-net: support inner header hash
On Fri, May 12, 2023 at 02:00:19PM +0800, Heng Qi wrote: > On Thu, May 11, 2023 at 02:22:12AM -0400, Michael S. Tsirkin wrote: > > On Wed, May 10, 2023 at 05:15:37PM +0800, Heng Qi wrote: > > > > > > > > > 在 2023/5/9 下午11:15, Michael S. Tsirkin 写道: > > > > On Tue, May 09, 2023 at 10:22:19PM +0800, Heng Qi wrote: > > > > > > > > > > 在 2023/5/5 下午10:56, Michael S. Tsirkin 写道: > > > > > > On Fri, May 05, 2023 at 09:51:15PM +0800, Heng Qi wrote: > > > > > > > On Thu, Apr 27, 2023 at 01:13:29PM -0400, Michael S. Tsirkin > > > > > > > wrote: > > > > > > > > On Thu, Apr 27, 2023 at 10:28:29AM +0800, Heng Qi wrote: > > > > > > > > > 在 2023/4/26 下午10:48, Michael S. Tsirkin 写道: > > > > > > > > > > On Wed, Apr 26, 2023 at 10:14:30PM +0800, Heng Qi wrote: > > > > > > > > > > > This does not mean that every device needs to implement > > > > > > > > > > > and support all of > > > > > > > > > > > these, they can choose to support some protocols they > > > > > > > > > > > want. > > > > > > > > > > > > > > > > > > > > > > I add these because we have scale application scenarios > > > > > > > > > > > for modern protocols > > > > > > > > > > > VXLAN-GPE/GENEVE: > > > > > > > > > > > > > > > > > > > > > > +\item In scenarios where the same flow passing through > > > > > > > > > > > different tunnels is expected to be received in the same > > > > > > > > > > > queue, > > > > > > > > > > > + warm caches, lessing locking, etc. are optimized > > > > > > > > > > > to obtain receiving performance. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Maybe the legacy GRE, VXLAN-GPE and GENEVE? But it has a > > > > > > > > > > > little crossover. > > > > > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > But VXLAN-GPE/GENEVE can use source port for entropy. > > > > > > > > > > > > > > > > > > > > It is recommended that the UDP source port number > > > > > > > > > > be calculated using a hash of fields from the inner > > > > > > > > > > packet > > > > > > > > > > > > > > > > > > > > That is best because > > > > > > > > > > it allows end to end control and is protocol agnostic. > > > > > > > > > Yes. I agree with this, I don't think we have an argument on > > > > > > > > > this point > > > > > > > > > right now.:) > > > > > > > > > > > > > > > > > > For VXLAN-GPE/GENEVE or other modern tunneling protocols, we > > > > > > > > > have to deal > > > > > > > > > with > > > > > > > > > scenarios where the same flow passes through different > > > > > > > > > tunnels. > > > > > > > > > > > > > > > > > > Having them hashed to the same rx queue, is hard to do via > > > > > > > > > outer headers. > > > > > > > > > > All that is missing is symmetric Toepliz and all is well? > > > > > > > > > The scenarios above or in the commit log also require inner > > > > > > > > > headers. > > > > > > > > Hmm I am not sure I get it 100%. > > > > > > > > Could you show an example with inner header hash in the port #, > > > > > > > > hash is symmetric, and you still have trouble? > > > > > > > > > > > > > > > > > > > > > > > > It kinds of sounds like not enough entropy is not the problem > > > > > > > > at this point. > > > > > > > Sorry for the late reply. :) > > > > > > > > > > > > > > For modern tunneling protocols, yes. > > > > > > > > > > > > > > > You now want to drop everything from the header > > > > > > > > except the UDP source port. Is that a fair summary? > > > > > > > > > > > > > > > For example, for the same flow passing through different VXLAN > > > > > > > tunnels, > > > > > > > packets in this flow have the same inner header and different > > > > > > > outer > > > > > > > headers. Sometimes these packets of the flow need to be hashed to > > > > > > > the > > > > > > > same rxq, then we can use the inner header as the hash input. > > > > > > > > > > > > > > Thanks! > > > > > > So, they will have the same source port yes? > > > > > Yes. The outer source port can be calculated using the 5-tuple of the > > > > > original packet, > > > > > and the outer ports are the same but the outer IPs are different after > > > > > different directions of the same flow pass through different tunnels. > > > > > > Any way to use that > > > > > We use it in monitoring, firewall and other scenarios. > > > > > > > > > > > so we don't depend on a specific protocol? > > > > > Yes, selected tunneling protocols can be used in this scenario like > > > > > this. > > > > > > > > > > Thanks. > > > > > > > > > No, the question was - can we generalize this somehow then? > > > > For example, a flag to ignore source IP when hashing? > > > > Or maybe just for UDP packets? > > > > > > 1. I think the common solution is based on the inner header, so that > > > GRE/IPIP tunnels can also enjoy inner symmetric hashing. > > > > > > 2. The VXLAN spec does not show that the outer source port in both > > > directions of the same flow must be the same [1] > > > (although the outer source port is calculated based
[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH v13] virtio-net: support inner header hash
On Thu, May 11, 2023 at 02:22:12AM -0400, Michael S. Tsirkin wrote: > On Wed, May 10, 2023 at 05:15:37PM +0800, Heng Qi wrote: > > > > > > 在 2023/5/9 下午11:15, Michael S. Tsirkin 写道: > > > On Tue, May 09, 2023 at 10:22:19PM +0800, Heng Qi wrote: > > > > > > > > 在 2023/5/5 下午10:56, Michael S. Tsirkin 写道: > > > > > On Fri, May 05, 2023 at 09:51:15PM +0800, Heng Qi wrote: > > > > > > On Thu, Apr 27, 2023 at 01:13:29PM -0400, Michael S. Tsirkin wrote: > > > > > > > On Thu, Apr 27, 2023 at 10:28:29AM +0800, Heng Qi wrote: > > > > > > > > 在 2023/4/26 下午10:48, Michael S. Tsirkin 写道: > > > > > > > > > On Wed, Apr 26, 2023 at 10:14:30PM +0800, Heng Qi wrote: > > > > > > > > > > This does not mean that every device needs to implement and > > > > > > > > > > support all of > > > > > > > > > > these, they can choose to support some protocols they want. > > > > > > > > > > > > > > > > > > > > I add these because we have scale application scenarios for > > > > > > > > > > modern protocols > > > > > > > > > > VXLAN-GPE/GENEVE: > > > > > > > > > > > > > > > > > > > > +\item In scenarios where the same flow passing through > > > > > > > > > > different tunnels is expected to be received in the same > > > > > > > > > > queue, > > > > > > > > > > + warm caches, lessing locking, etc. are optimized to > > > > > > > > > > obtain receiving performance. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Maybe the legacy GRE, VXLAN-GPE and GENEVE? But it has a > > > > > > > > > > little crossover. > > > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > But VXLAN-GPE/GENEVE can use source port for entropy. > > > > > > > > > > > > > > > > > > It is recommended that the UDP source port number > > > > > > > > >be calculated using a hash of fields from the inner > > > > > > > > > packet > > > > > > > > > > > > > > > > > > That is best because > > > > > > > > > it allows end to end control and is protocol agnostic. > > > > > > > > Yes. I agree with this, I don't think we have an argument on > > > > > > > > this point > > > > > > > > right now.:) > > > > > > > > > > > > > > > > For VXLAN-GPE/GENEVE or other modern tunneling protocols, we > > > > > > > > have to deal > > > > > > > > with > > > > > > > > scenarios where the same flow passes through different tunnels. > > > > > > > > > > > > > > > > Having them hashed to the same rx queue, is hard to do via > > > > > > > > outer headers. > > > > > > > > > All that is missing is symmetric Toepliz and all is well? > > > > > > > > The scenarios above or in the commit log also require inner > > > > > > > > headers. > > > > > > > Hmm I am not sure I get it 100%. > > > > > > > Could you show an example with inner header hash in the port #, > > > > > > > hash is symmetric, and you still have trouble? > > > > > > > > > > > > > > > > > > > > > It kinds of sounds like not enough entropy is not the problem > > > > > > > at this point. > > > > > > Sorry for the late reply. :) > > > > > > > > > > > > For modern tunneling protocols, yes. > > > > > > > > > > > > > You now want to drop everything from the header > > > > > > > except the UDP source port. Is that a fair summary? > > > > > > > > > > > > > For example, for the same flow passing through different VXLAN > > > > > > tunnels, > > > > > > packets in this flow have the same inner header and different outer > > > > > > headers. Sometimes these packets of the flow need to be hashed to > > > > > > the > > > > > > same rxq, then we can use the inner header as the hash input. > > > > > > > > > > > > Thanks! > > > > > So, they will have the same source port yes? > > > > Yes. The outer source port can be calculated using the 5-tuple of the > > > > original packet, > > > > and the outer ports are the same but the outer IPs are different after > > > > different directions of the same flow pass through different tunnels. > > > > > Any way to use that > > > > We use it in monitoring, firewall and other scenarios. > > > > > > > > > so we don't depend on a specific protocol? > > > > Yes, selected tunneling protocols can be used in this scenario like > > > > this. > > > > > > > > Thanks. > > > > > > > No, the question was - can we generalize this somehow then? > > > For example, a flag to ignore source IP when hashing? > > > Or maybe just for UDP packets? > > > > 1. I think the common solution is based on the inner header, so that > > GRE/IPIP tunnels can also enjoy inner symmetric hashing. > > > > 2. The VXLAN spec does not show that the outer source port in both > > directions of the same flow must be the same [1] > > (although the outer source port is calculated based on the consistent hash > > in the kernel. The consistent hash will sort the five-tuple before > > calculating hashing), > > but it is best not to assume that consistent hashing is used in all VXLAN > > implementations. > > I agree, best not to assume if it's not in the spec. > The requirement to hash two sides to same
