| Message ID | 20190801205149.4114-3-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | 40GbE Intel Wired LAN Driver Updates 2019-08-01 | expand |
On 8/1/19 1:51 PM, Jeff Kirsher wrote: > From: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > > This patch makes changed VM mac address visible on host via > ip link show command. This problem is fixed by copying last > unicast mac filter to vf->default_lan_addr.addr. Without > this patch if VF MAC was not set from host side and > if you run ip link show $pf, on host side you'd always > see a zero MAC, not the real VF MAC that VF assigned to > itself. > > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > --- > drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > index 02b09a8ad54c..21f7ac514d1f 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > @@ -2629,6 +2629,9 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg) > } else { > vf->num_mac++; > } > + if (is_valid_ether_addr(al->list[i].addr)) > + ether_addr_copy(vf->default_lan_addr.addr, > + al->list[i].addr); > } > } > spin_unlock_bh(&vsi->mac_filter_hash_lock); Since this copy is done inside the for-loop, it looks like you are copying every address in the list, not just the last one. This seems wasteful and unnecessary. Since it is possible, altho' unlikely, that the filter sync that happens a little later could fail, might it be better to do the copy after you know that the sync has succeeded? Why is the last mac chosen for display rather than the first? Is there anything special about the last mac as opposed to the first mac? sln
Good day Nelson In 99% cases VF has _only one_ unicast mac anyway, and the last MAC has been chosen because of VF mac address change algo - it marks unicast filter for deletion and appends a new unicast filter to the list. The implementation has been chosen because of simplicity /* Just 3 more lines to solve the issue */, from one point it may look wasteful for some 1% of VF corner cases. But from another point of view, more complicated code will affect 99% normal cases. Modern cpu are sensitive to cache thrash by code size and pipeline stalls by conditionals. Alex -----Original Message----- From: Shannon Nelson [mailto:snelson@pensando.io] Sent: Friday, August 2, 2019 2:11 AM To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; davem@davemloft.net Cc: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; netdev@vger.kernel.org; nhorman@redhat.com; sassmann@redhat.com; Bowers, AndrewX <andrewx.bowers@intel.com> Subject: Re: [net-next 2/9] i40e: make visible changed vf mac on host On 8/1/19 1:51 PM, Jeff Kirsher wrote: > From: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > > This patch makes changed VM mac address visible on host via ip link > show command. This problem is fixed by copying last unicast mac filter > to vf->default_lan_addr.addr. Without this patch if VF MAC was not set > from host side and if you run ip link show $pf, on host side you'd > always see a zero MAC, not the real VF MAC that VF assigned to itself. > > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > --- > drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > index 02b09a8ad54c..21f7ac514d1f 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > @@ -2629,6 +2629,9 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg) > } else { > vf->num_mac++; > } > + if (is_valid_ether_addr(al->list[i].addr)) > + ether_addr_copy(vf->default_lan_addr.addr, > + al->list[i].addr); > } > } > spin_unlock_bh(&vsi->mac_filter_hash_lock); Since this copy is done inside the for-loop, it looks like you are copying every address in the list, not just the last one. This seems wasteful and unnecessary. Since it is possible, altho' unlikely, that the filter sync that happens a little later could fail, might it be better to do the copy after you know that the sync has succeeded? Why is the last mac chosen for display rather than the first? Is there anything special about the last mac as opposed to the first mac? sln -------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN. Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
On 8/2/19 1:14 AM, Loktionov, Aleksandr wrote: > Good day Nelson Please don't top post. The custom on this mailing list is to answer inline in order to be sure we're answering in context. As it is, I believe you missed answering one of my questions. > In 99% cases VF has _only one_ unicast mac anyway, and the last MAC has been chosen because of VF mac address change algo - it marks unicast filter for deletion and appends a new unicast filter to the list. > The implementation has been chosen because of simplicity /* Just 3 more lines to solve the issue */, from one point it may look wasteful for some 1% of VF corner cases. > But from another point of view, more complicated code will affect 99% normal cases. Modern cpu are sensitive to cache thrash by code size and pipeline stalls by conditionals Yes, absolutely. So it follows that (a) we don't want to leave things in a loop if not necessary to repeat them, (b) we'd like to keep loops small as possible, (c) we want to keep our spin_lock sections small, and (d) we don't want to do things that later don't matter if an error happens when writing to the firmware. So it seems to me that you should move that copy line from the loop and outside of the spin_lock, and put it after the call sync the filters. Perhaps track the good mac index with "good_mac = i" at the end of the loop code and use that later to know which mac to copy into the vf struct. I also noticed that you're checking the mac addresses for validity, but only before copying it to the local vf struct. If you need to check the addresses, shouldn't you check them before you add them to the vf's filter list so you don't try to sync bad addresses to the FW? If the sync to the FW fails, you send the error status to the VF but you still have this new address copied into the vf struct. I think the copy line should be after the FW sync, and only if the sync succeeds. sln > > Alex > > -----Original Message----- > From: Shannon Nelson [mailto:snelson@pensando.io] > Sent: Friday, August 2, 2019 2:11 AM > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; davem@davemloft.net > Cc: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; netdev@vger.kernel.org; nhorman@redhat.com; sassmann@redhat.com; Bowers, AndrewX <andrewx.bowers@intel.com> > Subject: Re: [net-next 2/9] i40e: make visible changed vf mac on host > > On 8/1/19 1:51 PM, Jeff Kirsher wrote: >> From: Aleksandr Loktionov <aleksandr.loktionov@intel.com> >> >> This patch makes changed VM mac address visible on host via ip link >> show command. This problem is fixed by copying last unicast mac filter >> to vf->default_lan_addr.addr. Without this patch if VF MAC was not set >> from host side and if you run ip link show $pf, on host side you'd >> always see a zero MAC, not the real VF MAC that VF assigned to itself. >> >> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> >> Tested-by: Andrew Bowers <andrewx.bowers@intel.com> >> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >> --- >> drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c >> b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c >> index 02b09a8ad54c..21f7ac514d1f 100644 >> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c >> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c >> @@ -2629,6 +2629,9 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg) >> } else { >> vf->num_mac++; >> } >> + if (is_valid_ether_addr(al->list[i].addr)) >> + ether_addr_copy(vf->default_lan_addr.addr, >> + al->list[i].addr); >> } >> } >> spin_unlock_bh(&vsi->mac_filter_hash_lock); > Since this copy is done inside the for-loop, it looks like you are copying every address in the list, not just the last one. This seems wasteful and unnecessary. > > Since it is possible, altho' unlikely, that the filter sync that happens a little later could fail, might it be better to do the copy after you know that the sync has succeeded? > > Why is the last mac chosen for display rather than the first? Is there anything special about the last mac as opposed to the first mac? > > sln >
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c index 02b09a8ad54c..21f7ac514d1f 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c @@ -2629,6 +2629,9 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg) } else { vf->num_mac++; } + if (is_valid_ether_addr(al->list[i].addr)) + ether_addr_copy(vf->default_lan_addr.addr, + al->list[i].addr); } } spin_unlock_bh(&vsi->mac_filter_hash_lock);