Re: [PATCH net-next v1 2/3] virtio_transport_common: Set sibling VMs flag on the receive path

[Stefano Garzarella]

On Tue, Dec 01, 2020 at 09:01:05PM +0200, Paraschiv, Andra-Irina wrote:
> On 01/12/2020 18:22, Stefano Garzarella wrote:
> > On Tue, Dec 01, 2020 at 05:25:04PM +0200, Andra Paraschiv wrote:
> > > The vsock flag can be set during the connect() setup logic, when
> > > initializing the vsock address data structure variable. Then the vsock
> > > transport is assigned, also considering this flag.
> > >
> > > The vsock transport is also assigned on the (listen) receive path. The
> > > flag needs to be set considering the use case.
> > >
> > > Set the vsock flag of the remote address to the one targeted for sibling
> > > VMs communication if the following conditions are met:
> > >
> > > * The source CID of the packet is higher than VMADDR_CID_HOST.
> > > * The destination CID of the packet is higher than VMADDR_CID_HOST.
> > >
> > > Signed-off-by: Andra Paraschiv <andraprs@xxxxxxxxxx>
> > > ---
> > > net/vmw_vsock/virtio_transport_common.c | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/net/vmw_vsock/virtio_transport_common.c 
> > > b/net/vmw_vsock/virtio_transport_common.c
> > > index 5956939eebb78..871c84e0916b1 100644
> > > --- a/net/vmw_vsock/virtio_transport_common.c
> > > +++ b/net/vmw_vsock/virtio_transport_common.c
> > > @@ -1062,6 +1062,14 @@ virtio_transport_recv_listen(struct sock 
> > > *sk, struct virtio_vsock_pkt *pkt,
> > >       vsock_addr_init(&vchild->remote_addr, 
> > > le64_to_cpu(pkt->hdr.src_cid),
> > >                       le32_to_cpu(pkt->hdr.src_port));
> >
> > Maybe is better to create an helper function that other transports can
> > use for the same purpose or we can put this code in the
> > vsock_assign_transport() and set this flag only when the 'psk' argument
> > is not NULL (this is the case when it's called by the transports when we
> > receive a new connection request and 'psk' is the listener socket).
> >
> > The second way should allow us to support all the transports without
> > touching them.
>
> Ack, I was wondering about the other transports such as vmci or hyperv.
>
> I can move the logic below in the codebase that assigns the transport, 
> after checking 'psk'.
>
> > > +      /* If the packet is coming with the source and destination 
> > > CIDs higher
> > > +       * than VMADDR_CID_HOST, then a vsock channel should be 
> > > established for
> > > +       * sibling VMs communication.
> > > +       */
> > > +      if (vchild->local_addr.svm_cid > VMADDR_CID_HOST &&
> > > +          vchild->remote_addr.svm_cid > VMADDR_CID_HOST)
> > > +              vchild->remote_addr.svm_flag = 
> > > VMADDR_FLAG_SIBLING_VMS_COMMUNICATION;
> >
> > svm_flag is always initialized to 0 in vsock_addr_init(), so this
> > assignment is the first one and it's okay, but to avoid future issues
> > I'd use |= here to set the flag.
>
> Fair point. I was thinking more towards exclusive flags values 
> (purposes), but that's fine with the bitwise operator if we would get 
> a set of flag values together. I will also update the field name to 
> 'svm_flags', let me know if we should keep the previous one or there 
> is a better option.

Yeah, maybe in the future we will add some new flags and we'll only need 
to add them without touching this code.

Agree with the new 'svm_flags' field name.

Thanks,
Stefano