On Thu, Mar 16, 2017 at 10:18:21AM +0100, Maxime Coquelin wrote:
>
>
> On 03/16/2017 08:08 AM, Yuanhan Liu wrote:
> >On Tue, Mar 14, 2017 at 10:53:23AM +0100, Maxime Coquelin wrote:
> >>
> >>
> >>On 03/14/2017 10:46 AM, Maxime Coquelin wrote:
> >>>
> >>>
> >>>On 03/03/2017 10:51 AM, Yuanhan Liu wrote:
> >>>>Introduce few APIs to set/get/enable/disable driver features.
> >>>>
> >>>>Signed-off-by: Yuanhan Liu <yuanhan....@linux.intel.com>
> >>>>---
> >>>>lib/librte_vhost/rte_vhost_version.map | 10 ++++
> >>>>lib/librte_vhost/rte_virtio_net.h | 9 ++++
> >>>>lib/librte_vhost/socket.c | 90
> >>>>++++++++++++++++++++++++++++++++++
> >>>>3 files changed, 109 insertions(+)
> >>>
> >>>Nice!
> >>>Reviewed-by: Maxime Coquelin <maxime.coque...@redhat.com>
> >>>
> >>>I wonder whether we could protect from setting/enabling/disabling
> >>>features once negotiation is done?
> >
> >Those APIs won't be able to change the negotiated features. They are
> >just some interfaces before the vhost-user connection is established.
>
> Right.
> I meant it could return an error is the connection is already
> established. Else, the caller might think the feature has been
> successfully enabled/disabled, whereas it is not the case.
> But this is maybe over-engineering to handle this case.
>
> >Ideally, we could/should get rid of the enabling/disabling functions:
> >let the vhost-user driver to handle the features directly (say, for
> >vhost-user pmd, we could use vdev options to disable/enable few specific
> >features). Once that is done, use the rte_vhost_driver_set_features()
> >set the features once.
>
> Ok, but what about vhost lib users and TSO (I'm thinking of OVS).
Yes, that's why I kept them :)
--yliu
>
> >The reason I introduced the enable/disable_features() is to keep the
> >compatability with the builtin vhost-user net driver (virtio_net.c).
> >If there is a plan to move it into vhost-pmd, they won't be needed.
> >And I don't think that will happen soon.
>
> I agree with you that would be ideal, but unlikely to happen soon.
>
> >>Oh, I forgot one comment on this patch.
> >>Since these new functions are part to the API, shouldn't them be
> >>documented?
> >
> >Yes, indeed, it's also noted in my cover letter.
>
> Oops, I missed that!
>
> Thanks,
> Maxime
> > --yliu
> >
