From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752670AbdGSCkB (ORCPT ); Tue, 18 Jul 2017 22:40:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34276 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751560AbdGSCj7 (ORCPT ); Tue, 18 Jul 2017 22:39:59 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 62510285B5 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jasowang@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 62510285B5 Subject: Re: [PATCH net-next 5/5] virtio-net: switch off offloads on demand if possible on XDP set To: "Michael S. Tsirkin" Cc: virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org References: <1500295441-19339-1-git-send-email-jasowang@redhat.com> <1500295441-19339-6-git-send-email-jasowang@redhat.com> <20170718225011-mutt-send-email-mst@kernel.org> From: Jason Wang Message-ID: <494ca887-1df7-6f72-0f2d-16a70a141cbc@redhat.com> Date: Wed, 19 Jul 2017 10:39:53 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170718225011-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Wed, 19 Jul 2017 02:39:59 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017年07月19日 04:07, Michael S. Tsirkin wrote: > On Mon, Jul 17, 2017 at 08:44:01PM +0800, Jason Wang wrote: >> Current XDP implementation want guest offloads feature to be disabled > s/want/wants/ > >> on qemu cli. > on the device. > >> This is inconvenient and means guest can't benefit from >> offloads if XDP is not used. This patch tries to address this >> limitation by disable > disabling > >> the offloads on demand through control guest >> offloads. Guest offloads will be disabled and enabled on demand on XDP >> set. >> >> Signed-off-by: Jason Wang > In fact, since we no longer reset when XDP is set, > here device might have offloads enabled, buffers are > used but not consumed, then XDP is set. > > This can result in > - packet scattered across multiple buffers > (handled correctly but need to update the comment) Ok. > - packet may have VIRTIO_NET_HDR_F_NEEDS_CSUM, in that case > the spec says "The checksum on the packet is incomplete". > (probably needs to be handled by calculating the checksum). That's an option. Maybe it's tricky but I was thinking whether or not we can just keep the CHECKSUM_PARTIAL here. > > > Ideas for follow-up patches: > > - skip looking at packet data completely > won't work if you play with checksums dynamically > but can be done if disabled on device > - allow ethtools to tweak offloads from userspace as well Right. Thanks > >> --- >> drivers/net/virtio_net.c | 70 ++++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 65 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >> index e732bd6..d970c2d 100644 >> --- a/drivers/net/virtio_net.c >> +++ b/drivers/net/virtio_net.c >> @@ -57,6 +57,11 @@ DECLARE_EWMA(pkt_len, 0, 64) >> >> #define VIRTNET_DRIVER_VERSION "1.0.0" >> >> +const unsigned long guest_offloads[] = { VIRTIO_NET_F_GUEST_TSO4, >> + VIRTIO_NET_F_GUEST_TSO6, >> + VIRTIO_NET_F_GUEST_ECN, >> + VIRTIO_NET_F_GUEST_UFO }; >> + >> struct virtnet_stats { >> struct u64_stats_sync tx_syncp; >> struct u64_stats_sync rx_syncp; >> @@ -164,10 +169,13 @@ struct virtnet_info { >> u8 ctrl_promisc; >> u8 ctrl_allmulti; >> u16 ctrl_vid; >> + u64 ctrl_offloads; >> >> /* Ethtool settings */ >> u8 duplex; >> u32 speed; >> + >> + unsigned long guest_offloads; >> }; >> >> struct padded_vnet_hdr { >> @@ -1889,6 +1897,47 @@ static int virtnet_restore_up(struct virtio_device *vdev) >> return err; >> } >> >> +static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads) >> +{ >> + struct scatterlist sg; >> + vi->ctrl_offloads = cpu_to_virtio64(vi->vdev, offloads); >> + >> + sg_init_one(&sg, &vi->ctrl_offloads, sizeof(vi->ctrl_offloads)); >> + >> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_GUEST_OFFLOADS, >> + VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, &sg)) { >> + dev_warn(&vi->dev->dev, "Fail to set guest offload. \n"); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int virtnet_clear_guest_offloads(struct virtnet_info *vi) >> +{ >> + u64 offloads = 0; >> + >> + if (!vi->guest_offloads) >> + return 0; >> + >> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM)) >> + offloads = 1ULL << VIRTIO_NET_F_GUEST_CSUM; >> + >> + return virtnet_set_guest_offloads(vi, offloads); >> +} >> + >> +static int virtnet_restore_guest_offloads(struct virtnet_info *vi) >> +{ >> + u64 offloads = vi->guest_offloads; >> + >> + if (!vi->guest_offloads) >> + return 0; >> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM)) >> + offloads |= 1ULL << VIRTIO_NET_F_GUEST_CSUM; >> + >> + return virtnet_set_guest_offloads(vi, offloads); >> +} >> + >> static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, >> struct netlink_ext_ack *extack) >> { >> @@ -1898,10 +1947,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, >> u16 xdp_qp = 0, curr_qp; >> int i, err; >> >> - if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) || >> - virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) || >> - virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) || >> - virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO)) { >> + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) >> + && (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) || >> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) || >> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) || >> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO))) { >> NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing LRO, disable LRO first"); >> return -EOPNOTSUPP; >> } >> @@ -1950,6 +2000,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, >> for (i = 0; i < vi->max_queue_pairs; i++) { >> old_prog = rtnl_dereference(vi->rq[i].xdp_prog); >> rcu_assign_pointer(vi->rq[i].xdp_prog, prog); >> + if (i == 0) { >> + if (!old_prog) >> + virtnet_clear_guest_offloads(vi); >> + if (!prog) >> + virtnet_restore_guest_offloads(vi); >> + } >> if (old_prog) >> bpf_prog_put(old_prog); >> napi_enable(&vi->rq[i].napi); >> @@ -2583,6 +2639,10 @@ static int virtnet_probe(struct virtio_device *vdev) >> netif_carrier_on(dev); >> } >> >> + for (i = 0; i < ARRAY_SIZE(guest_offloads); i++) >> + if (virtio_has_feature(vi->vdev, guest_offloads[i])) >> + set_bit(guest_offloads[i], &vi->guest_offloads); >> + >> pr_debug("virtnet: registered device %s with %d RX and TX vq's\n", >> dev->name, max_queue_pairs); >> >> @@ -2679,7 +2739,7 @@ static struct virtio_device_id id_table[] = { >> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \ >> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \ >> VIRTIO_NET_F_CTRL_MAC_ADDR, \ >> - VIRTIO_NET_F_MTU >> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS >> >> static unsigned int features[] = { >> VIRTNET_FEATURES, >> -- >> 2.7.4