| Message ID | 2f2560f749186c0eb1055f9926f464587e419eeb.1688051252.git.yin31149@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Vhost-vdpa Shadow Virtqueue _F_CTRL_RX commands support | expand |
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. 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 >
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? 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 >> >
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 > >> > > >
On 2023/7/5 14:29, Eugenio Perez Martin wrote: > 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; > } > ... Thanks for your explanation. I will refactor the patch according to your suggestion. Thanks! > > 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 >>>> >>> >> >
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) { + /* + * 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; }
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(+)