[0/2] vhost: add port mirroring function in the vhost lib

  Similar to the port mirroring function on the switch or router, this
patch set implements such function on the Vhost lib. When
data is sent to a front-end, it will also send the data to its mirror
front-end. When data is received from a front-end, it will also send
the data to its mirror front-end.

Cheng Jiang (2):
  vhost: add ingress API for port mirroring datapath
  vhost: add egress API for port mirroring datapath

 lib/vhost/rte_vhost_async.h |   17 +
 lib/vhost/version.map       |    3 +
 lib/vhost/virtio_net.c      | 1266 +++++++++++++++++++++++++++++++++++
 3 files changed, 1286 insertions(+)

--
2.35.1
  

Hi Cheng,

On 4/21/23 03:09, Cheng Jiang wrote:
> Similar to the port mirroring function on the switch or router, this
> patch set implements such function on the Vhost lib. When
> data is sent to a front-end, it will also send the data to its mirror
> front-end. When data is received from a front-end, it will also send
> the data to its mirror front-end.

Why not just keeping mirroring in the switch/router?
I am really not convinced this is the way to go:
1. API is too complex
2. It requires async support
3. There is too much code duplication, it increases  virtio-net.c by
    30%, and it is without packed ring support.
4. If mirror port is down for any reason, packets to/from the original
    port are dropped.
5. It seems to assume negotiated features of the two ports are
    identical, e.g. Virtio-net header length? If so, that's not a
    manageable solution.

Regards,
Maxime

> 
> Cheng Jiang (2):
>    vhost: add ingress API for port mirroring datapath
>    vhost: add egress API for port mirroring datapath
> 
>   lib/vhost/rte_vhost_async.h |   17 +
>   lib/vhost/version.map       |    3 +
>   lib/vhost/virtio_net.c      | 1266 +++++++++++++++++++++++++++++++++++
>   3 files changed, 1286 insertions(+)
> 
> --
> 2.35.1
>
  

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, May 3, 2023 5:37 PM
> To: Jiang, Cheng1 <cheng1.jiang@intel.com>; Xia, Chenbo
> <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Ding, Xuan
> <xuan.ding@intel.com>; Ma, WenwuX <wenwux.ma@intel.com>; Wang, YuanX
> <yuanx.wang@intel.com>; He, Xingguang <xingguang.he@intel.com>; David
> Marchand <david.marchand@redhat.com>
> Subject: Re: [PATCH 0/2] vhost: add port mirroring function in the vhost lib
> 
> Hi Cheng,
> 
> On 4/21/23 03:09, Cheng Jiang wrote:
> > Similar to the port mirroring function on the switch or router, this
> > patch set implements such function on the Vhost lib. When data is sent
> > to a front-end, it will also send the data to its mirror front-end.
> > When data is received from a front-end, it will also send the data to
> > its mirror front-end.
> 
> Why not just keeping mirroring in the switch/router?
> I am really not convinced this is the way to go:
> 1. API is too complex
> 2. It requires async support
> 3. There is too much code duplication, it increases  virtio-net.c by
>     30%, and it is without packed ring support.
> 4. If mirror port is down for any reason, packets to/from the original
>     port are dropped.
> 5. It seems to assume negotiated features of the two ports are
>     identical, e.g. Virtio-net header length? If so, that's not a
>     manageable solution.

Thank you for your feedback.
I concur that placing the mirror function in the Vhost library is not ideal. We are currently considering implementing either a mirror Vhost PMD, or adding a function to TestPMD to handle this functionality.
Would you please share your thoughts on this plan and let us know which option you prefer?

Thanks a lot,
Cheng



> 
> Regards,
> Maxime
> 
> >
> > Cheng Jiang (2):
> >    vhost: add ingress API for port mirroring datapath
> >    vhost: add egress API for port mirroring datapath
> >
> >   lib/vhost/rte_vhost_async.h |   17 +
> >   lib/vhost/version.map       |    3 +
> >   lib/vhost/virtio_net.c      | 1266 +++++++++++++++++++++++++++++++++++
> >   3 files changed, 1286 insertions(+)
> >
> > --
> > 2.35.1
> >
  

> -----Original Message-----
> From: Jiang, Cheng1 <cheng1.jiang@intel.com>
> Sent: Monday, May 8, 2023 8:23 PM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Xia, Chenbo
> <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Ding, Xuan
> <xuan.ding@intel.com>; Ma, WenwuX <wenwux.ma@intel.com>; Wang, YuanX
> <yuanx.wang@intel.com>; He, Xingguang <xingguang.he@intel.com>; David
> Marchand <david.marchand@redhat.com>
> Subject: RE: [PATCH 0/2] vhost: add port mirroring function in the vhost
> lib
> 
> Hi Maxime,
> 
> > -----Original Message-----
> > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Sent: Wednesday, May 3, 2023 5:37 PM
> > To: Jiang, Cheng1 <cheng1.jiang@intel.com>; Xia, Chenbo
> > <chenbo.xia@intel.com>
> > Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Ding, Xuan
> > <xuan.ding@intel.com>; Ma, WenwuX <wenwux.ma@intel.com>; Wang, YuanX
> > <yuanx.wang@intel.com>; He, Xingguang <xingguang.he@intel.com>; David
> > Marchand <david.marchand@redhat.com>
> > Subject: Re: [PATCH 0/2] vhost: add port mirroring function in the vhost
> lib
> >
> > Hi Cheng,
> >
> > On 4/21/23 03:09, Cheng Jiang wrote:
> > > Similar to the port mirroring function on the switch or router, this
> > > patch set implements such function on the Vhost lib. When data is sent
> > > to a front-end, it will also send the data to its mirror front-end.
> > > When data is received from a front-end, it will also send the data to
> > > its mirror front-end.
> >
> > Why not just keeping mirroring in the switch/router?
> > I am really not convinced this is the way to go:
> > 1. API is too complex
> > 2. It requires async support
> > 3. There is too much code duplication, it increases  virtio-net.c by
> >     30%, and it is without packed ring support.
> > 4. If mirror port is down for any reason, packets to/from the original
> >     port are dropped.
> > 5. It seems to assume negotiated features of the two ports are
> >     identical, e.g. Virtio-net header length? If so, that's not a
> >     manageable solution.
> 
> Thank you for your feedback.
> I concur that placing the mirror function in the Vhost library is not
> ideal. We are currently considering implementing either a mirror Vhost PMD,
> or adding a function to TestPMD to handle this functionality.
> Would you please share your thoughts on this plan and let us know which
> option you prefer?

Based on current implementation, it seems that vhost lib could be ignorant
of the mirroring usage. Making these logic into APP like testpmd seems to make
more sense.

Thanks,
Chenbo

> 
> Thanks a lot,
> Cheng
> 
> 
> 
> >
> > Regards,
> > Maxime
> >
> > >
> > > Cheng Jiang (2):
> > >    vhost: add ingress API for port mirroring datapath
> > >    vhost: add egress API for port mirroring datapath
> > >
> > >   lib/vhost/rte_vhost_async.h |   17 +
> > >   lib/vhost/version.map       |    3 +
> > >   lib/vhost/virtio_net.c      | 1266
> +++++++++++++++++++++++++++++++++++
> > >   3 files changed, 1286 insertions(+)
> > >
> > > --
> > > 2.35.1
> > >
  

On 5/11/23 10:59, Xia, Chenbo wrote:
>> -----Original Message-----
>> From: Jiang, Cheng1 <cheng1.jiang@intel.com>
>> Sent: Monday, May 8, 2023 8:23 PM
>> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Xia, Chenbo
>> <chenbo.xia@intel.com>
>> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Ding, Xuan
>> <xuan.ding@intel.com>; Ma, WenwuX <wenwux.ma@intel.com>; Wang, YuanX
>> <yuanx.wang@intel.com>; He, Xingguang <xingguang.he@intel.com>; David
>> Marchand <david.marchand@redhat.com>
>> Subject: RE: [PATCH 0/2] vhost: add port mirroring function in the vhost
>> lib
>>
>> Hi Maxime,
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Sent: Wednesday, May 3, 2023 5:37 PM
>>> To: Jiang, Cheng1 <cheng1.jiang@intel.com>; Xia, Chenbo
>>> <chenbo.xia@intel.com>
>>> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Ding, Xuan
>>> <xuan.ding@intel.com>; Ma, WenwuX <wenwux.ma@intel.com>; Wang, YuanX
>>> <yuanx.wang@intel.com>; He, Xingguang <xingguang.he@intel.com>; David
>>> Marchand <david.marchand@redhat.com>
>>> Subject: Re: [PATCH 0/2] vhost: add port mirroring function in the vhost
>> lib
>>>
>>> Hi Cheng,
>>>
>>> On 4/21/23 03:09, Cheng Jiang wrote:
>>>> Similar to the port mirroring function on the switch or router, this
>>>> patch set implements such function on the Vhost lib. When data is sent
>>>> to a front-end, it will also send the data to its mirror front-end.
>>>> When data is received from a front-end, it will also send the data to
>>>> its mirror front-end.
>>>
>>> Why not just keeping mirroring in the switch/router?
>>> I am really not convinced this is the way to go:
>>> 1. API is too complex
>>> 2. It requires async support
>>> 3. There is too much code duplication, it increases  virtio-net.c by
>>>      30%, and it is without packed ring support.
>>> 4. If mirror port is down for any reason, packets to/from the original
>>>      port are dropped.
>>> 5. It seems to assume negotiated features of the two ports are
>>>      identical, e.g. Virtio-net header length? If so, that's not a
>>>      manageable solution.
>>
>> Thank you for your feedback.
>> I concur that placing the mirror function in the Vhost library is not
>> ideal. We are currently considering implementing either a mirror Vhost PMD,
>> or adding a function to TestPMD to handle this functionality.
>> Would you please share your thoughts on this plan and let us know which
>> option you prefer?
> 
> Based on current implementation, it seems that vhost lib could be ignorant
> of the mirroring usage. Making these logic into APP like testpmd seems to make
> more sense.

I agree with Chenbo, it should be done at the application level, which
would enable to also mirror non-Vhost ports.

Thanks,
Maxime

> Thanks,
> Chenbo
> 
>>
>> Thanks a lot,
>> Cheng
>>
>>
>>
>>>
>>> Regards,
>>> Maxime
>>>
>>>>
>>>> Cheng Jiang (2):
>>>>     vhost: add ingress API for port mirroring datapath
>>>>     vhost: add egress API for port mirroring datapath
>>>>
>>>>    lib/vhost/rte_vhost_async.h |   17 +
>>>>    lib/vhost/version.map       |    3 +
>>>>    lib/vhost/virtio_net.c      | 1266
>> +++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 1286 insertions(+)
>>>>
>>>> --
>>>> 2.35.1
>>>>
>
  

Hi Maxime/Chenbo,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, May 11, 2023 8:16 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; Jiang, Cheng1
> <cheng1.jiang@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Ding, Xuan
> <xuan.ding@intel.com>; Ma, WenwuX <wenwux.ma@intel.com>; Wang, YuanX
> <yuanx.wang@intel.com>; He, Xingguang <xingguang.he@intel.com>; David
> Marchand <david.marchand@redhat.com>
> Subject: Re: [PATCH 0/2] vhost: add port mirroring function in the vhost lib
> 
> 
> 
> On 5/11/23 10:59, Xia, Chenbo wrote:
> >> -----Original Message-----
> >> From: Jiang, Cheng1 <cheng1.jiang@intel.com>
> >> Sent: Monday, May 8, 2023 8:23 PM
> >> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Xia, Chenbo
> >> <chenbo.xia@intel.com>
> >> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Ding, Xuan
> >> <xuan.ding@intel.com>; Ma, WenwuX <wenwux.ma@intel.com>; Wang,
> YuanX
> >> <yuanx.wang@intel.com>; He, Xingguang <xingguang.he@intel.com>; David
> >> Marchand <david.marchand@redhat.com>
> >> Subject: RE: [PATCH 0/2] vhost: add port mirroring function in the
> >> vhost lib
> >>
> >> Hi Maxime,
> >>
> >>> -----Original Message-----
> >>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>> Sent: Wednesday, May 3, 2023 5:37 PM
> >>> To: Jiang, Cheng1 <cheng1.jiang@intel.com>; Xia, Chenbo
> >>> <chenbo.xia@intel.com>
> >>> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Ding, Xuan
> >>> <xuan.ding@intel.com>; Ma, WenwuX <wenwux.ma@intel.com>; Wang,
> YuanX
> >>> <yuanx.wang@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> >>> David Marchand <david.marchand@redhat.com>
> >>> Subject: Re: [PATCH 0/2] vhost: add port mirroring function in the
> >>> vhost
> >> lib
> >>>
> >>> Hi Cheng,
> >>>
> >>> On 4/21/23 03:09, Cheng Jiang wrote:
> >>>> Similar to the port mirroring function on the switch or router,
> >>>> this patch set implements such function on the Vhost lib. When data
> >>>> is sent to a front-end, it will also send the data to its mirror front-end.
> >>>> When data is received from a front-end, it will also send the data
> >>>> to its mirror front-end.
> >>>
> >>> Why not just keeping mirroring in the switch/router?
> >>> I am really not convinced this is the way to go:
> >>> 1. API is too complex
> >>> 2. It requires async support
> >>> 3. There is too much code duplication, it increases  virtio-net.c by
> >>>      30%, and it is without packed ring support.
> >>> 4. If mirror port is down for any reason, packets to/from the original
> >>>      port are dropped.
> >>> 5. It seems to assume negotiated features of the two ports are
> >>>      identical, e.g. Virtio-net header length? If so, that's not a
> >>>      manageable solution.
> >>
> >> Thank you for your feedback.
> >> I concur that placing the mirror function in the Vhost library is not
> >> ideal. We are currently considering implementing either a mirror
> >> Vhost PMD, or adding a function to TestPMD to handle this functionality.
> >> Would you please share your thoughts on this plan and let us know
> >> which option you prefer?
> >
> > Based on current implementation, it seems that vhost lib could be
> > ignorant of the mirroring usage. Making these logic into APP like
> > testpmd seems to make more sense.
> 
> I agree with Chenbo, it should be done at the application level, which would
> enable to also mirror non-Vhost ports.

I mostly agree with your opinions. However, the workload for implementing a general mirror framework to support all ports in the application level would be too large for us for now. Currently, we are more focused on demonstrating the superior performance advantages of async Vhost and DSA with this mirror work. Therefore, we may need to temporarily put this upstream process on hold.

I greatly appreciate your feedback and help. Maybe we can discuss the details in the future. Thank you again!

Best regards,
Cheng

> 
> Thanks,
> Maxime
> 
> > Thanks,
> > Chenbo
> >
> >>
> >> Thanks a lot,
> >> Cheng
> >>
> >>
> >>
> >>>
> >>> Regards,
> >>> Maxime
> >>>
> >>>>
> >>>> Cheng Jiang (2):
> >>>>     vhost: add ingress API for port mirroring datapath
> >>>>     vhost: add egress API for port mirroring datapath
> >>>>
> >>>>    lib/vhost/rte_vhost_async.h |   17 +
> >>>>    lib/vhost/version.map       |    3 +
> >>>>    lib/vhost/virtio_net.c      | 1266
> >> +++++++++++++++++++++++++++++++++++
> >>>>    3 files changed, 1286 insertions(+)
> >>>>
> >>>> --
> >>>> 2.35.1
> >>>>
> >