Re: [PATCH RFC net-next v5 07/14] virtio/vsock: add common datagram send path

[Bobby Eshleman]

On Thu, Jul 27, 2023 at 10:57:05AM +0300, Arseniy Krasnov wrote:
> 
> 
> On 26.07.2023 20:08, Bobby Eshleman wrote:
> > On Sat, Jul 22, 2023 at 11:16:05AM +0300, Arseniy Krasnov wrote:
> >>
> >>
> >> On 19.07.2023 03:50, Bobby Eshleman wrote:
> >>> This commit implements the common function
> >>> virtio_transport_dgram_enqueue for enqueueing datagrams. It does not add
> >>> usage in either vhost or virtio yet.
> >>>
> >>> Signed-off-by: Bobby Eshleman <bobby.eshleman@xxxxxxxxxxxxx>
> >>> ---
> >>>  net/vmw_vsock/virtio_transport_common.c | 76 ++++++++++++++++++++++++++++++++-
> >>>  1 file changed, 75 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> >>> index ffcbdd77feaa..3bfaff758433 100644
> >>> --- a/net/vmw_vsock/virtio_transport_common.c
> >>> +++ b/net/vmw_vsock/virtio_transport_common.c
> >>> @@ -819,7 +819,81 @@ virtio_transport_dgram_enqueue(struct vsock_sock *vsk,
> >>>  			       struct msghdr *msg,
> >>>  			       size_t dgram_len)
> >>>  {
> >>> -	return -EOPNOTSUPP;
> >>> +	/* Here we are only using the info struct to retain style uniformity
> >>> +	 * and to ease future refactoring and merging.
> >>> +	 */
> >>> +	struct virtio_vsock_pkt_info info_stack = {
> >>> +		.op = VIRTIO_VSOCK_OP_RW,
> >>> +		.msg = msg,
> >>> +		.vsk = vsk,
> >>> +		.type = VIRTIO_VSOCK_TYPE_DGRAM,
> >>> +	};
> >>> +	const struct virtio_transport *t_ops;
> >>> +	struct virtio_vsock_pkt_info *info;
> >>> +	struct sock *sk = sk_vsock(vsk);
> >>> +	struct virtio_vsock_hdr *hdr;
> >>> +	u32 src_cid, src_port;
> >>> +	struct sk_buff *skb;
> >>> +	void *payload;
> >>> +	int noblock;
> >>> +	int err;
> >>> +
> >>> +	info = &info_stack;
> >>
> >> I think 'info' assignment could be moved below, to the place where it is used
> >> first time.
> >>
> >>> +
> >>> +	if (dgram_len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE)
> >>> +		return -EMSGSIZE;
> >>> +
> >>> +	t_ops = virtio_transport_get_ops(vsk);
> >>> +	if (unlikely(!t_ops))
> >>> +		return -EFAULT;
> >>> +
> >>> +	/* Unlike some of our other sending functions, this function is not
> >>> +	 * intended for use without a msghdr.
> >>> +	 */
> >>> +	if (WARN_ONCE(!msg, "vsock dgram bug: no msghdr found for dgram enqueue\n"))
> >>> +		return -EFAULT;
> >>
> >> Sorry, but is that possible? I thought 'msg' is always provided by general socket layer (e.g. before
> >> af_vsock.c code) and can't be NULL for DGRAM. Please correct me if i'm wrong.
> >>
> >> Also I see, that in af_vsock.c , 'vsock_dgram_sendmsg()' dereferences 'msg' for checking MSG_OOB without any
> >> checks (before calling transport callback - this function in case of virtio). So I think if we want to keep
> >> this type of check - such check must be placed in af_vsock.c or somewhere before first dereference of this pointer.
> >>
> > 
> > There is some talk about dgram sockets adding additional messages types
> > in the future that help with congestion control. Those messages won't
> > come from the socket layer, so msghdr will be null. Since there is no
> > other function for sending datagrams, it seemed likely that this
> > function would be reworked for that purpose. I felt that adding this
> > check was a direct way to make it explicit that this function is
> > currently designed only for the socket-layer caller.
> > 
> > Perhaps a comment would suffice?
> 
> I see, thanks, it is for future usage. Sorry for dumb question: but if msg is NULL, how
> we will decide what to do in this call? Interface of this callback will be updated or
> some fields of 'vsock_sock' will contain type of such messages ?
> 
> Thanks, Arseniy
> 

Hey Arseniy, sorry about the delay I forgot about this chunk of the
thread.

This warning was intended to help by calling attention to the fact that
even though this function is the only way to send dgram packets, unlike
the connectible sending function virtio_transport_send_pkt_info() this
actually requires a non-NULL msg... it seems like it doesn't help and
just causes more confusion than anything. It is a wasted cycle on the
fastpath too, so I think I'll just drop it in the next rev.

> > 
> >>> +
> >>> +	noblock = msg->msg_flags & MSG_DONTWAIT;
> >>> +
> >>> +	/* Use sock_alloc_send_skb to throttle by sk_sndbuf. This helps avoid
> >>> +	 * triggering the OOM.
> >>> +	 */
> >>> +	skb = sock_alloc_send_skb(sk, dgram_len + VIRTIO_VSOCK_SKB_HEADROOM,
> >>> +				  noblock, &err);
> >>> +	if (!skb)
> >>> +		return err;
> >>> +
> >>> +	skb_reserve(skb, VIRTIO_VSOCK_SKB_HEADROOM);
> >>> +
> >>> +	src_cid = t_ops->transport.get_local_cid();
> >>> +	src_port = vsk->local_addr.svm_port;
> >>> +
> >>> +	hdr = virtio_vsock_hdr(skb);
> >>> +	hdr->type	= cpu_to_le16(info->type);
> >>> +	hdr->op		= cpu_to_le16(info->op);
> >>> +	hdr->src_cid	= cpu_to_le64(src_cid);
> >>> +	hdr->dst_cid	= cpu_to_le64(remote_addr->svm_cid);
> >>> +	hdr->src_port	= cpu_to_le32(src_port);
> >>> +	hdr->dst_port	= cpu_to_le32(remote_addr->svm_port);
> >>> +	hdr->flags	= cpu_to_le32(info->flags);
> >>> +	hdr->len	= cpu_to_le32(dgram_len);
> >>> +
> >>> +	skb_set_owner_w(skb, sk);
> >>> +
> >>> +	payload = skb_put(skb, dgram_len);
> >>> +	err = memcpy_from_msg(payload, msg, dgram_len);
> >>> +	if (err)
> >>> +		return err;
> >>
> >> Do we need free allocated skb here ?
> >>
> > 
> > Yep, thanks.
> > 
> >>> +
> >>> +	trace_virtio_transport_alloc_pkt(src_cid, src_port,
> >>> +					 remote_addr->svm_cid,
> >>> +					 remote_addr->svm_port,
> >>> +					 dgram_len,
> >>> +					 info->type,
> >>> +					 info->op,
> >>> +					 0);
> >>> +
> >>> +	return t_ops->send_pkt(skb);
> >>>  }
> >>>  EXPORT_SYMBOL_GPL(virtio_transport_dgram_enqueue);
> >>>  
> >>>
> >>
> >> Thanks, Arseniy
> > 
> > Thanks for the review!
> > 
> > Best,
> > Bobby