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

  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@bytedance.com>
---
 net/vmw_vsock/virtio_transport_common.c | 76 ++++++++++++++++++++++++++++++++-
 1 file changed, 75 insertions(+), 1 deletion(-)
  

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@bytedance.com>
> ---
>  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.

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

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

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@bytedance.com>
> > ---
> >  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?

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

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@bytedance.com>
>>> ---
>>>  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

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

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@bytedance.com>
> >>> ---
> >>>  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