[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [Proposal] Relationship between XDP and rx-csum in virtio-nety
å 2023/5/24 äå3:24, Heng Qi åé:
On Wed, May 24, 2023 at 01:52:30PM +0800, Jason Wang wrote:On Wed, May 24, 2023 at 1:07âPM Heng Qi <hengqi@linux.alibaba.com> wrote:On Wed, May 24, 2023 at 12:09:22PM +0800, Jason Wang wrote:On Tue, May 23, 2023 at 9:51âPM Heng Qi <hengqi@linux.alibaba.com> wrote:On Tue, May 23, 2023 at 09:30:28AM -0400, Michael S. Tsirkin wrote:On Tue, May 23, 2023 at 05:18:20PM +0800, Heng Qi wrote:On Tue, May 23, 2023 at 03:15:37AM -0400, Michael S. Tsirkin wrote:On Tue, May 23, 2023 at 10:41:18AM +0800, Heng Qi wrote:On Mon, May 22, 2023 at 03:10:05PM -0400, Michael S. Tsirkin wrote:On Mon, May 22, 2023 at 08:12:00PM +0800, Heng Qi wrote:1) Add a feature bit to the virtio specification to tell the sender that a fully csumed packet must be sent.Who is the sender in this picture? The driver?The device or the driver. When the device is hw, the sender is more likely to be a device. When the device is sw, the sender can be a device or a driver. But in general, this feature is inclined to constrain the behavior of the device and the driver from the receiving side.Based on above I am guessing you are talking about driver getting packets from device, I wish you used terms from virtio spec.Yes, I'm going to use the terminology of the virtio spec.For example: VIRTIO_NET_F_UNNECESSARY_CSUM : The driver tells the device that you must send me a fully csumed packet. Then the specific implementation can be (1) the sender sends a fully csumed packet; (2) the receiver receives a CHECKSUM_PARTIAL packet, and the device helps calculate the fully csum (because the two parties in the communication are located on the same host, the packet is trusted.). In summary, if VIRTIO_NET_F_UNNECESSARY_CSUM is negotiated, the driver will no longer receive any packets marked CHECKSUM_PARTIAL. Thanks.This is what clearing VIRTIO_NET_F_GUEST_CSUM does.Yes, but with VIRTIO_NET_F_GUEST_CSUM cleared, although the device can receive a fully checksummed packet, we can no longer enjoy the device's ability to validate the packet checksum. That is, the value of \field{flags} in the virtio_net_hdr structure is set to 0, which means that the packet received by the driver will not be marked as VIRTIO_NET_HDR_F_DATA_VALID. So, we need a feature bit (let's say VIRTIO_NET_F_UNNECESSARY_CSUM). If VIRTIO_NET_F_UNNECESSARY_CSUM is negotiated, the device must give the driver a fully checksummed packet, and the packet is validated by the device with \field{flags} set to VIRTIO_NET_HDR_F_DATA_VALID.I feel you are trying to say that clearing VIRTIO_NET_F_GUEST_CSUM disables all offloads but you want to keep some of them?No, what I mean is that a feature VIRTIO_NET_F_UNNECESSARY_CSUM is needed in addition to VIRTIO_NET_F_GUEST_CSUM, if both features are negotiated, then the driver may always receive packets marked as VIRTIO_NET_HDR_F_DATA_VALID, which means that we can now load XDP at the same time.Makes no sense to me. VIRTIO_NET_F_GUEST_CSUM set already allows VIRTIO_NET_HDR_F_DATA_VALID:We need to focus on what happens when the XDP program is loaded: The current virtio-net needs to turn off the VIRTIO_NET_F_GUEST_CSUM feature when loading XDP. The main reason for doing this is because VIRTIO_NET_F_GUEST_CSUM allows to receive packets marked as VIRTIO_NET_HDR_F_NEEDS_CSUM. Such packets are not compatible with XDP programs, because we cannot guarantee that the csum_{start, offset} fields are correct after XDP modifies the packets.We can try to fix or workaround this. A rough idea is for example by using a flow dissector?How can we fix this with a flow dissector? Could you please explain in detail?I mean you can use a flow dissector to probe the csum_start and csum_offset.I think this is a good workaround. We save virtio-net-hdr first, and after running XDP, if the flag of saved virtio-net-hdr contains VIRTIO_NET_HDR_F_NEEDS_CSUM, we'll probe the csum_{start, offset}.
Hi, Jason.Do we choose this option first as an attempt to solve the problem described by this proposal?
If so, I'll try to make a patch. Thanks.
Anyhow the GSO of virtio-net was marked as dodgy which means the csum_start/offset needs to be validated again in the xmit path. Or we can monitor if the packet is modified or not, if not, we don't need to zero vnet header?Yes, this is a way, and we've thought about this solution. A relatively simple solution is to add a new flag to the XDP program (through XDP core layer), which indicates that the XDP program is a read-only XDP program (ie, does not modify the packet).XDP used to have something like this. Maybe we can re add them.Has XDP core removed the similar flag? I'm missing something from the past, can you give me a link to it please?Then to load such an XDP program we no longer need to filter out the VIRTIO_NET_F_GUEST_CSUM feature. But why we didn't propose this solution in the 'Proposal', because it seems that only virtio-net has encountered related problems. Virtual network cards like veth, even if they receive partial csumed packets (corresponding to virtio-net's packets marked as VIRTIO_NET_HDR_F_NEEDS_CSUM), they still open NETIF_F_RXCSUM (corresponding to virtio-net's VIRTIO_NET_F_GUEST_CSUM). So there is another trade-off: the current virtnet_xdp_set() no longer filters out VIRTIO_NET_F_GUEST_CSUM. The reasons I think are the following: 1. Referring to vethâs way, the XDP program is more of a monitoring/firewall, and may not modify data packets.Probably not, XDP could be used for building tunnels.Yes, opening VIRTIO_NET_F_GUEST_CSUM directly for XDP programs, is a tradeoff and may cause packet loss, but we can also benefit from rx hw csum offloading.2. We can still use XDP for packets marked as VIRTIO_NET_HDR_F_DATA_VALID.Not sure I get this but who is going to adjust csum_start/offset?The information for csum_start/offset may be overwritten or may no longer be correct. For example, XDP stores some new data in the packet headroom and overwrites csum_{start, offset}; or pushes new data in front of the original header. Thanks.3. Packets converted from xdp_{buff, frame} may not be ip_forwarded (because ip_forward sysctl is disabled by default)That's suboptimal. ThanksThanks.ThanksSo in order for the driver to continue to enjoy the device's ability to validate packets while XDP is loaded, we need a new feature to tell the device not to deliver packets marked as VIRTIO_NET_HDR_F_NEEDS_CSUM (in other words, the device can only deliver packets marked as VIRTIO_NET_HDR_F_DATA_VALID). Thanks.\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can be set: if so, the packet checksum at offset \field{csum_offset} from \field{csum_start} and any preceding checksums have been validated. The checksum on the packet is incomplete and if bit VIRTIO_NET_HDR_F_RSC_INFO is not set in \field{flags}, then \field{csum_start} and \field{csum_offset} indicate how to calculate it (see Packet Transmission point 1). Did you maybe mean if either feature is negotiated?Again please use virtio terminology not Linux. to help you out, in current linux, VIRTIO_NET_HDR_F_NEEDS_CSUM and VIRTIO_NET_HDR_F_DATA_VALID will set CHECKSUM_PARTIAL and CHECKSUM_UNNECESSARY respectively.Sure. Will do as you suggested. Thanks.-- MST --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.orgThis publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/--------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.orgThis publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]