Re: [PATCH RFC v2 2/4] vdpa: Restore MAC address filtering state

[Eugenio Perez Martin]

On Wed, Jul 5, 2023 at 3:43 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> On 2023/7/4 22:53, Eugenio Perez Martin wrote:
> > On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
> >>
> >> This patch refactors vhost_vdpa_net_load_mac() to
> >> restore the MAC address filtering state at device's startup.
> >>
> >> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> >> ---
> >> v2:
> >>    - use iovec suggested by Eugenio
> >>    - avoid sending CVQ command in default state
> >>
> >> v1: 
> >> https://lore.kernel.org/all/00f72fe154a882fd6dc15bc39e3a1ac63f9dadce.1687402580.git.yin31149@gmail.com/
> >>
> >>   net/vhost-vdpa.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 51 insertions(+)
> >>
> >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >> index 0bd1c7817c..cb45c84c88 100644
> >> --- a/net/vhost-vdpa.c
> >> +++ b/net/vhost-vdpa.c
> >> @@ -665,6 +665,57 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, 
> >> const VirtIONet *n)
> >>           }
> >>       }
> >>
> >> +    if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
> >> +        if (n->mac_table.in_use != 0) {
> >
> > This may be just style nitpicking, but I find it more clear to return
> > early if conditions are not met and then send the CVQ command.
>
> Yes, this makes code more clear to read.
>
> But it appears that we may meet a problem if the function
> vhost_vdpa_net_load_x() sends multiple CVQ commands. It is possible that
> we might not meet the condition for one of the CVQ commands, but we
> could still meet the conditions for other CVQ commands.
>
> Therefore, in the case of vhost_vdpa_net_load_x() sending multiple CVQ
> commands, if we still hope to use this style, should we split the
> function into multiple functions, with each function responsible for
> sending only one CVQ command? Or should we jump to the next CVQ command
> instead of returning from the function if the conditions are not met?
>

In that case I'd suggest using multiples if() {}, as each ctrl command
processing code is very small.

But for VIRTIO_NET_F_CTRL_RX particular case your patch propose:
if (x) {
  if (y) {
    ...
  }
}

So in my opinion it makes sense to convert to:
if (!x || !y) {
  return;
}
...

We can always change later if needed.

Thanks!

> Thanks!
>
>
> > Something like:
> > /*
> >   * According to ...
> >   */
> > if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) ||
> > (n->mac_table.in_use == 0)) {
> >    return 0
> > }
> >
> > uni_entries = n->mac_table.first_multi,
> > ...
> > ---
> >
> > Now I just realized vhost_vdpa_net_load_mac does not follow this for
> > checking VIRTIO_NET_F_CTRL_MAC_ADDR.
> >
> > I'm ok if you leave it this way though.
> >
> > Thanks!
> >
> >> +            /*
> >> +             * According to virtio_net_reset(), device uses an empty MAC 
> >> filter
> >> +             * table as its default state.
> >> +             *
> >> +             * Therefore, there is no need to send this CVQ command if the
> >> +             * driver also sets an empty MAC filter table, which aligns 
> >> with
> >> +             * the device's defaults.
> >> +             *
> >> +             * Note that the device's defaults can mismatch the driver's
> >> +             * configuration only at live migration.
> >> +             */
> >> +            uint32_t uni_entries = n->mac_table.first_multi,
> >> +                     uni_macs_size = uni_entries * ETH_ALEN,
> >> +                     mul_entries = n->mac_table.in_use - uni_entries,
> >> +                     mul_macs_size = mul_entries * ETH_ALEN;
> >> +            struct virtio_net_ctrl_mac uni = {
> >> +                .entries = cpu_to_le32(uni_entries),
> >> +            };
> >> +            struct virtio_net_ctrl_mac mul = {
> >> +                .entries = cpu_to_le32(mul_entries),
> >> +            };
> >> +            const struct iovec data[] = {
> >> +                {
> >> +                    .iov_base = &uni,
> >> +                    .iov_len = sizeof(uni),
> >> +                }, {
> >> +                    .iov_base = n->mac_table.macs,
> >> +                    .iov_len = uni_macs_size,
> >> +                }, {
> >> +                    .iov_base = &mul,
> >> +                    .iov_len = sizeof(mul),
> >> +                }, {
> >> +                    .iov_base = &n->mac_table.macs[uni_macs_size],
> >> +                    .iov_len = mul_macs_size,
> >> +                },
> >> +            };
> >> +            ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
> >> +                                        VIRTIO_NET_CTRL_MAC,
> >> +                                        VIRTIO_NET_CTRL_MAC_TABLE_SET,
> >> +                                        data, ARRAY_SIZE(data));
> >> +            if (unlikely(dev_written < 0)) {
> >> +                return dev_written;
> >> +            }
> >> +            if (*s->status != VIRTIO_NET_OK) {
> >> +                return -EINVAL;
> >> +            }
> >> +        }
> >> +    }
> >> +
> >>       return 0;
> >>   }
> >>
> >> --
> >> 2.25.1
> >>
> >
>