Re: [PATCH net-next 5/5] virtio-net: switch off offloads on demand if possible on XDP set

[Michael S. Tsirkin]

On Wed, Jul 19, 2017 at 10:39:53AM +0800, Jason Wang wrote:
> 
> 
> 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 <jasowang@xxxxxxxxxx>
> > 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.

XDP programs do not expect this currently. As it's a temporary
condition, let's just fix it up.

> > 
> > 
> > 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