| Message ID | 20200819134349.22129-2-sameehj@amazon.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Enhance current features in ena driver | expand |
On Wed, Aug 19, 2020 at 01:43:46PM +0000, sameehj@amazon.com wrote: > From: Sameeh Jubran <sameehj@amazon.com> > > unsigned long is the type for doing maths on pointers. Maths on pointers is perfectly valid. The real issue here is you have all your types mixed up. > - ptr = (u64 *)((uintptr_t)&ring->tx_stats + > - (uintptr_t)ena_stats->stat_offset); > + ptr = (u64 *)((unsigned long)&ring->tx_stats + > + ena_stats->stat_offset); struct ena_ring { ... union { struct ena_stats_tx tx_stats; struct ena_stats_rx rx_stats; }; struct ena_stats_tx { u64 cnt; u64 bytes; u64 queue_stop; u64 prepare_ctx_err; u64 queue_wakeup; ... } &ring->tx_stats will give you a struct *ena_stats_tx. Arithmetic on that, adding 1 for example, takes you forward a full ena_stats_tx structure. Not what you want. &ring->tx_stats.cnt however, will give you a u64 *. Adding 1 to that will give you bytes, etc. Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Wednesday, August 19, 2020 5:17 PM > To: Jubran, Samih <sameehj@amazon.com> > Cc: davem@davemloft.net; netdev@vger.kernel.org; Woodhouse, David > <dwmw@amazon.co.uk>; Machulsky, Zorik <zorik@amazon.com>; > Matushevsky, Alexander <matua@amazon.com>; Bshara, Saeed > <saeedb@amazon.com>; Wilson, Matt <msw@amazon.com>; Liguori, > Anthony <aliguori@amazon.com>; Bshara, Nafea <nafea@amazon.com>; > Tzalik, Guy <gtzalik@amazon.com>; Belgazal, Netanel > <netanel@amazon.com>; Saidi, Ali <alisaidi@amazon.com>; Herrenschmidt, > Benjamin <benh@amazon.com>; Kiyanovski, Arthur > <akiyano@amazon.com>; Dagan, Noam <ndagan@amazon.com> > Subject: RE: [EXTERNAL] [PATCH V2 net-next 1/4] net: ena: ethtool: use > unsigned long for pointer arithmetics > > CAUTION: This email originated from outside of the organization. Do not click > links or open attachments unless you can confirm the sender and know the > content is safe. > > > > On Wed, Aug 19, 2020 at 01:43:46PM +0000, sameehj@amazon.com wrote: > > From: Sameeh Jubran <sameehj@amazon.com> > > > > unsigned long is the type for doing maths on pointers. > > Maths on pointers is perfectly valid. The real issue here is you have all your > types mixed up. The stat_offset field has the bytes from the start of the struct, the math is perfectly valid IMO¸ I have also went for the extra step and tested it using prints. > > > - ptr = (u64 *)((uintptr_t)&ring->tx_stats + > > - (uintptr_t)ena_stats->stat_offset); > > + ptr = (u64 *)((unsigned long)&ring->tx_stats + > > + ena_stats->stat_offset); > > struct ena_ring { > ... > union { > struct ena_stats_tx tx_stats; > struct ena_stats_rx rx_stats; > }; > > struct ena_stats_tx { > u64 cnt; > u64 bytes; > u64 queue_stop; > u64 prepare_ctx_err; > u64 queue_wakeup; > ... > } > > &ring->tx_stats will give you a struct *ena_stats_tx. Arithmetic on that, > adding 1 for example, takes you forward a full ena_stats_tx structure. Not > what you want. > > &ring->tx_stats.cnt however, will give you a u64 *. Adding 1 to that will give > you bytes, etc. If I understand you well, the alternative approach you are suggesting is: ptr = &ring->tx_stats.cnt + ena_stats->stat_offset; of course we need to convert the stat_offset field to be in 8 bytes resolution instead. This approach has a potential bug hidden in it. If in the future someone decides to expand the "ena_stats_tx" struct and add a field preceding cnt, cnt will no longer be the beginning of the struct, which will cause a bug." Therefore, if you have another way to do this, please share it. Otherwise I'd rather leave this code as it is for the sake of robustness. > > Andrew
> -----Original Message----- > From: Jubran, Samih > Sent: Thursday, August 20, 2020 3:13 PM > To: 'Andrew Lunn' <andrew@lunn.ch> > Cc: davem@davemloft.net; netdev@vger.kernel.org; Woodhouse, David > <dwmw@amazon.co.uk>; Machulsky, Zorik <zorik@amazon.com>; > Matushevsky, Alexander <matua@amazon.com>; Bshara, Saeed > <saeedb@amazon.com>; Wilson, Matt <msw@amazon.com>; Liguori, > Anthony <aliguori@amazon.com>; Bshara, Nafea <nafea@amazon.com>; > Tzalik, Guy <gtzalik@amazon.com>; Belgazal, Netanel > <netanel@amazon.com>; Saidi, Ali <alisaidi@amazon.com>; Herrenschmidt, > Benjamin <benh@amazon.com>; Kiyanovski, Arthur > <akiyano@amazon.com>; Dagan, Noam <ndagan@amazon.com> > Subject: RE: [EXTERNAL] [PATCH V2 net-next 1/4] net: ena: ethtool: use > unsigned long for pointer arithmetics > > > > > -----Original Message----- > > From: Andrew Lunn <andrew@lunn.ch> > > Sent: Wednesday, August 19, 2020 5:17 PM > > To: Jubran, Samih <sameehj@amazon.com> > > Cc: davem@davemloft.net; netdev@vger.kernel.org; Woodhouse, David > > <dwmw@amazon.co.uk>; Machulsky, Zorik <zorik@amazon.com>; > Matushevsky, > > Alexander <matua@amazon.com>; Bshara, Saeed > <saeedb@amazon.com>; > > Wilson, Matt <msw@amazon.com>; Liguori, Anthony > <aliguori@amazon.com>; > > Bshara, Nafea <nafea@amazon.com>; Tzalik, Guy <gtzalik@amazon.com>; > > Belgazal, Netanel <netanel@amazon.com>; Saidi, Ali > > <alisaidi@amazon.com>; Herrenschmidt, Benjamin <benh@amazon.com>; > > Kiyanovski, Arthur <akiyano@amazon.com>; Dagan, Noam > > <ndagan@amazon.com> > > Subject: RE: [EXTERNAL] [PATCH V2 net-next 1/4] net: ena: ethtool: use > > unsigned long for pointer arithmetics > > > > CAUTION: This email originated from outside of the organization. Do > > not click links or open attachments unless you can confirm the sender > > and know the content is safe. > > > > > > > > On Wed, Aug 19, 2020 at 01:43:46PM +0000, sameehj@amazon.com wrote: > > > From: Sameeh Jubran <sameehj@amazon.com> > > > > > > unsigned long is the type for doing maths on pointers. > > > > Maths on pointers is perfectly valid. The real issue here is you have > > all your types mixed up. > > The stat_offset field has the bytes from the start of the struct, the math is > perfectly valid IMO¸ I have also went for the extra step and tested it using > prints. > > > > > > - ptr = (u64 *)((uintptr_t)&ring->tx_stats + > > > - (uintptr_t)ena_stats->stat_offset); > > > + ptr = (u64 *)((unsigned long)&ring->tx_stats + > > > + ena_stats->stat_offset); > > > > struct ena_ring { > > ... > > union { > > struct ena_stats_tx tx_stats; > > struct ena_stats_rx rx_stats; > > }; > > > > struct ena_stats_tx { > > u64 cnt; > > u64 bytes; > > u64 queue_stop; > > u64 prepare_ctx_err; > > u64 queue_wakeup; > > ... > > } > > > > &ring->tx_stats will give you a struct *ena_stats_tx. Arithmetic on > > that, adding 1 for example, takes you forward a full ena_stats_tx > > structure. Not what you want. > > > > &ring->tx_stats.cnt however, will give you a u64 *. Adding 1 to that > > will give you bytes, etc. > > > If I understand you well, the alternative approach you are suggesting is: > > ptr = &ring->tx_stats.cnt + ena_stats->stat_offset; > > of course we need to convert the stat_offset field to be in 8 bytes resolution > instead. > > This approach has a potential bug hidden in it. If in the future someone > decides to expand the "ena_stats_tx" struct and add a field preceding cnt, > cnt will no longer be the beginning of the struct, which will cause a bug." > > Therefore, if you have another way to do this, please share it. Otherwise I'd > rather leave this code as it is for the sake of robustness. > > > > > Andrew Ping.
On Thu, Aug 20, 2020 at 12:13:15PM +0000, Jubran, Samih wrote: > > > > -----Original Message----- > > From: Andrew Lunn <andrew@lunn.ch> > > Sent: Wednesday, August 19, 2020 5:17 PM > > To: Jubran, Samih <sameehj@amazon.com> > > Cc: davem@davemloft.net; netdev@vger.kernel.org; Woodhouse, David > > <dwmw@amazon.co.uk>; Machulsky, Zorik <zorik@amazon.com>; > > Matushevsky, Alexander <matua@amazon.com>; Bshara, Saeed > > <saeedb@amazon.com>; Wilson, Matt <msw@amazon.com>; Liguori, > > Anthony <aliguori@amazon.com>; Bshara, Nafea <nafea@amazon.com>; > > Tzalik, Guy <gtzalik@amazon.com>; Belgazal, Netanel > > <netanel@amazon.com>; Saidi, Ali <alisaidi@amazon.com>; Herrenschmidt, > > Benjamin <benh@amazon.com>; Kiyanovski, Arthur > > <akiyano@amazon.com>; Dagan, Noam <ndagan@amazon.com> > > Subject: RE: [EXTERNAL] [PATCH V2 net-next 1/4] net: ena: ethtool: use > > unsigned long for pointer arithmetics > > > > CAUTION: This email originated from outside of the organization. Do not click > > links or open attachments unless you can confirm the sender and know the > > content is safe. > > > > > > > > On Wed, Aug 19, 2020 at 01:43:46PM +0000, sameehj@amazon.com wrote: > > > From: Sameeh Jubran <sameehj@amazon.com> > > > > > > unsigned long is the type for doing maths on pointers. > > > > Maths on pointers is perfectly valid. The real issue here is you have all your > > types mixed up. > > The stat_offset field has the bytes from the start of the struct, the math is perfectly valid IMO¸ > I have also went for the extra step and tested it using prints. > > > > > > - ptr = (u64 *)((uintptr_t)&ring->tx_stats + > > > - (uintptr_t)ena_stats->stat_offset); > > > + ptr = (u64 *)((unsigned long)&ring->tx_stats + > > > + ena_stats->stat_offset); > > > > struct ena_ring { > > ... > > union { > > struct ena_stats_tx tx_stats; > > struct ena_stats_rx rx_stats; > > }; > > > > struct ena_stats_tx { > > u64 cnt; > > u64 bytes; > > u64 queue_stop; > > u64 prepare_ctx_err; > > u64 queue_wakeup; > > ... > > } > > > > &ring->tx_stats will give you a struct *ena_stats_tx. Arithmetic on that, > > adding 1 for example, takes you forward a full ena_stats_tx structure. Not > > what you want. > > > > &ring->tx_stats.cnt however, will give you a u64 *. Adding 1 to that will give > > you bytes, etc. > > > If I understand you well, the alternative approach you are suggesting is: > > ptr = &ring->tx_stats.cnt + ena_stats->stat_offset; I don't want to stir up the pot, but do you really need the offsetof() of each member in the stats struct? Couldn't you piggyback on assumption that these stats need to be u64 and just walk the struct with pointer? struct ena_ring *ring; int offset; int i, j; u8 *ptr; for (i = 0; i < adapter->num_io_queues; i++) { /* Tx stats */ ring = &adapter->tx_ring[i]; ptr = (u8 *)&ring->tx_stats; for (j = 0; j < ENA_STATS_ARRAY_TX; j++) { ena_safe_update_stat((u64 *)ptr, (*data)++, &ring->syncp); ptr += sizeof(u64); } } I find this as a simpler and lighter solution. There might be issues with code typed in email client, but you get the idea. > > of course we need to convert the stat_offset field to be in 8 bytes resolution instead. > > This approach has a potential bug hidden in it. If in the future > someone decides to expand the "ena_stats_tx" struct and add a field preceding cnt, > cnt will no longer be the beginning of the struct, which will cause a bug." > > Therefore, if you have another way to do this, please share it. Otherwise I'd > rather leave this code as it is for the sake of robustness. > > > > > Andrew
Maciej Fijalkowski <maciej.fijalkowski@intel.com> writes: > On Thu, Aug 20, 2020 at 12:13:15PM +0000, Jubran, Samih wrote: >> >> > ... >> > >> > On Wed, Aug 19, 2020 at 01:43:46PM +0000, sameehj@amazon.com >> > wrote: >> > > From: Sameeh Jubran <sameehj@amazon.com> >> > > >> > > unsigned long is the type for doing maths on pointers. >> > >> > Maths on pointers is perfectly valid. The real issue here is >> > you have all your >> > types mixed up. >> >> The stat_offset field has the bytes from the start of the >> struct, the math is perfectly valid IMO¸ >> I have also went for the extra step and tested it using prints. >> >> > >> > > - ptr = (u64 >> > > *)((uintptr_t)&ring->tx_stats + >> > > - >> > > (uintptr_t)ena_stats->stat_offset); >> > > + ptr = (u64 *)((unsigned >> > > long)&ring->tx_stats + >> > > + ena_stats->stat_offset); >> > >> > struct ena_ring { >> > ... >> > union { >> > struct ena_stats_tx tx_stats; >> > struct ena_stats_rx rx_stats; >> > }; >> > >> > struct ena_stats_tx { >> > u64 cnt; >> > u64 bytes; >> > u64 queue_stop; >> > u64 prepare_ctx_err; >> > u64 queue_wakeup; >> > ... >> > } >> > >> > &ring->tx_stats will give you a struct >> > *ena_stats_tx. Arithmetic on that, >> > adding 1 for example, takes you forward a full ena_stats_tx >> > structure. Not >> > what you want. >> > >> > &ring->tx_stats.cnt however, will give you a u64 *. Adding 1 >> > to that will give >> > you bytes, etc. >> >> >> If I understand you well, the alternative approach you are >> suggesting is: >> >> ptr = &ring->tx_stats.cnt + ena_stats->stat_offset; > > I don't want to stir up the pot, but do you really need the > offsetof() of > each member in the stats struct? Couldn't you piggyback on > assumption that > these stats need to be u64 and just walk the struct with > pointer? > > struct ena_ring *ring; > int offset; > int i, j; > u8 *ptr; > > for (i = 0; i < adapter->num_io_queues; i++) { > /* Tx stats */ > ring = &adapter->tx_ring[i]; > ptr = (u8 *)&ring->tx_stats; > > for (j = 0; j < ENA_STATS_ARRAY_TX; j++) { > ena_safe_update_stat((u64 *)ptr, > (*data)++, &ring->syncp); > ptr += sizeof(u64); > } > } > > I find this as a simpler and lighter solution. There might be > issues with > code typed in email client, but you get the idea. > >> >> of course we need to convert the stat_offset field to be in 8 >> bytes resolution instead. >> >> This approach has a potential bug hidden in it. If in the >> future >> someone decides to expand the "ena_stats_tx" struct and add a >> field preceding cnt, >> cnt will no longer be the beginning of the struct, which will >> cause a bug." >> >> Therefore, if you have another way to do this, please share >> it. Otherwise I'd >> rather leave this code as it is for the sake of robustness. >> >> > >> > Andrew Hi all, We tried to implement your suggestion, and found that removing the stat_offset field causes problems that are challenging to solve. Removing stat_offset introduces a requirement that the statistics in a stat strings array (check [1] for example) and stat variables struct (check [2] for example) must be in the same order. This requirement is prone to future bugs that might be challenging to locate. We also tried to unify the array and struct creation by using X macros. At the moment this change requires more time and effort by us and our customers need this code merged asap. [1] https://elixir.bootlin.com/linux/v5.9- rc3/source/drivers/net/ethernet/amazon/ena/ena_ethtool.c#L71 [2] https://elixir.bootlin.com/linux/v5.9- rc3/source/drivers/net/ethernet/amazon/ena/ena_netdev.h#L232 (This message was sent before but didn't seem to get into the mailing list. Apologies if you got it twice)
On Sun, 6 Sep 2020 13:47:13 +0300 Shay Agroskin wrote: > Maciej Fijalkowski <maciej.fijalkowski@intel.com> writes: > > I don't want to stir up the pot, but do you really need the > > offsetof() of > > each member in the stats struct? Couldn't you piggyback on > > assumption that > > these stats need to be u64 and just walk the struct with > > pointer? > > > > struct ena_ring *ring; > > int offset; > > int i, j; > > u8 *ptr; > > > > for (i = 0; i < adapter->num_io_queues; i++) { > > /* Tx stats */ > > ring = &adapter->tx_ring[i]; > > ptr = (u8 *)&ring->tx_stats; > > > > for (j = 0; j < ENA_STATS_ARRAY_TX; j++) { > > ena_safe_update_stat((u64 *)ptr, > > (*data)++, &ring->syncp); > > ptr += sizeof(u64); > > } > > } > > > > I find this as a simpler and lighter solution. There might be > > issues with > > code typed in email client, but you get the idea. > > > >> > >> of course we need to convert the stat_offset field to be in 8 > >> bytes resolution instead. > >> > >> This approach has a potential bug hidden in it. If in the > >> future > >> someone decides to expand the "ena_stats_tx" struct and add a > >> field preceding cnt, > >> cnt will no longer be the beginning of the struct, which will > >> cause a bug." > >> > >> Therefore, if you have another way to do this, please share > >> it. Otherwise I'd > >> rather leave this code as it is for the sake of robustness. > >> > >> > > >> > Andrew > > Hi all, > > We tried to implement your suggestion, and found that removing the > stat_offset > field causes problems that are challenging to solve. > Removing stat_offset introduces a requirement that the statistics > in a stat > strings array (check [1] for example) and stat variables struct > (check [2] for > example) must be in the same order. > This requirement is prone to future bugs that might be challenging > to locate. > We also tried to unify the array and struct creation by > using X macros. At the moment this change requires more time and > effort by us > and our customers need this code merged asap. > > [1] https://elixir.bootlin.com/linux/v5.9- > rc3/source/drivers/net/ethernet/amazon/ena/ena_ethtool.c#L71 > [2] https://elixir.bootlin.com/linux/v5.9- > rc3/source/drivers/net/ethernet/amazon/ena/ena_netdev.h#L232 > > (This message was sent before but didn't seem to get into the > mailing list. Apologies if you got it twice) Divide the offset by 8, cast &ring->tx_stats to u64 * (without referencing cnt). That should be fine.
diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c index 430275bc0..897beddc5 100644 --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c @@ -141,8 +141,8 @@ static void ena_queue_stats(struct ena_adapter *adapter, u64 **data) for (j = 0; j < ENA_STATS_ARRAY_TX; j++) { ena_stats = &ena_stats_tx_strings[j]; - ptr = (u64 *)((uintptr_t)&ring->tx_stats + - (uintptr_t)ena_stats->stat_offset); + ptr = (u64 *)((unsigned long)&ring->tx_stats + + ena_stats->stat_offset); ena_safe_update_stat(ptr, (*data)++, &ring->syncp); } @@ -153,8 +153,8 @@ static void ena_queue_stats(struct ena_adapter *adapter, u64 **data) for (j = 0; j < ENA_STATS_ARRAY_RX; j++) { ena_stats = &ena_stats_rx_strings[j]; - ptr = (u64 *)((uintptr_t)&ring->rx_stats + - (uintptr_t)ena_stats->stat_offset); + ptr = (u64 *)((unsigned long)&ring->rx_stats + + ena_stats->stat_offset); ena_safe_update_stat(ptr, (*data)++, &ring->syncp); } @@ -170,8 +170,8 @@ static void ena_dev_admin_queue_stats(struct ena_adapter *adapter, u64 **data) for (i = 0; i < ENA_STATS_ARRAY_ENA_COM; i++) { ena_stats = &ena_stats_ena_com_strings[i]; - ptr = (u64 *)((uintptr_t)&adapter->ena_dev->admin_queue.stats + - (uintptr_t)ena_stats->stat_offset); + ptr = (u64 *)((unsigned long)&adapter->ena_dev->admin_queue.stats + + ena_stats->stat_offset); *(*data)++ = *ptr; } @@ -189,8 +189,8 @@ static void ena_get_ethtool_stats(struct net_device *netdev, for (i = 0; i < ENA_STATS_ARRAY_GLOBAL; i++) { ena_stats = &ena_stats_global_strings[i]; - ptr = (u64 *)((uintptr_t)&adapter->dev_stats + - (uintptr_t)ena_stats->stat_offset); + ptr = (u64 *)((unsigned long)&adapter->dev_stats + + ena_stats->stat_offset); ena_safe_update_stat(ptr, data++, &adapter->syncp); }