RE: [EXT] [PATCH 6/7] virtio-net: Add flow filter requests

[Satananda Burla <sburla@marvell.com>]

Hi Parav,

> -----Original Message-----
> From: Parav Pandit <parav@nvidia.com>
> Sent: Sunday, October 1, 2023 3:58 AM
> To: Satananda Burla <sburla@marvell.com>; virtio-comment@lists.oasis-
> open.org; mst@redhat.com; cohuck@redhat.com
> Cc: Shahaf Shuler <shahafs@nvidia.com>; si-wei.liu@oracle.com;
> xuanzhuo@linux.alibaba.com; Heng Qi <hengqi@linux.alibaba.com>
> Subject: RE: [EXT] [PATCH 6/7] virtio-net: Add flow filter requests
> 
> 
> 
> > From: Satananda Burla <sburla@marvell.com>
> > Sent: Sunday, October 1, 2023 3:13 PM
> 
> 
> > > +struct virtio_net_ff_match_fields {
> > > +        le32 num_entries;
> > > +        struct virtio_net_ff_match_field match_entries[]; };
> > It is better to fix the network protocol order here.(l2,l3,l4).
> 
> It is recommended but I am not sure if we should enforce it.
> 
> How about writing it as order SHOULD follow the packet field order..
> This will cover across L2, L3 and within a given header type too.
Fine with me.
> 
> > > +
> > > +#define VIRTIO_NET_FF_DEST_RQ 0
> > > +
> > > +struct virtio_net_ff_dest {
> > > +        u8 dest_type;
> > > +        u8 reserved[3];
> > > +        union {
> > > +                le16 vq_index;
> > > +                le32 reserved1;
> > > +        };
> > > +};
> > > +
> > > +struct virtio_net_ff_action {
> > > +        u8 action;
> > > +        u8 reserved[7];
> > > +};
> > > +
> > > +#define VIRTIO_NET_FF_ACTION_DROP 0
> > > +#define VIRTIO_NET_FF_ACTION_FORWARD 1
> > > +
> > > +struct virtio_net_ff_add {
> > > +        u8 op;
> > > +        u8 priority;	/* higher the value, higher priority */
> > > +        u16 group_id;
> > > +        le32 id;
> > > +        struct virtio_net_ff_match_fields match;
> > > +        struct virtio_net_ff_dest dest;
> > > +        struct virtio_net_ff_action action;
> > Isn't dest also an action ? for instance, if ACTION is drop, there is no
> point in
> > adding destination, similarly if ACTION is forward, can we not add the
> vq_index
> > to the action itself using the reserved bytes? based on action type, the
> > additional bytes could be interpreted.
Any comment on this?
> > > +
> > > +        struct virtio_net_ff_match_fields mask;	/* optional */
> > How is the presence of this field indicated if it is optional ?
> > Does it need a field like has_mask?
> Yes. I missed it.
> 
> > Can this be not added into struct virtio_net_ff_match_field ?
> I think yes, the issue is not all use cases (like ARFS) needs mask, who
> always uses exact values.
> So communicating and setting mask is redundant (also the struct is bit
> large).
> 
> But I like your suggestion that we can have mask defined as, just
> match_value.
> Since value cannot live without its supporting type, finding it hard to
> have only value for the mask.
> 
> > The mask only applies to struct virtio_net_ff_match_value. So it may be
> better
> > to just use struct virtio_net_ff_match_field {
> > 	struct virtio_net_ff_match_type type;
> > 	struct virtio_net_ff_match_value value;
> > 	struct virtio_net_ff_match_value mask;
> > };
> > instead of repeating the type again.
> > > +
> How about encoding has_mask in the type, so that mask can be optional in
> the above struct you proposed?
> With that we get both the benefits of size saving and computation too.
Yes. Makes sense.