| Message ID | 1685080949-18316-1-git-send-email-shradhagupta@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] hv_netvsc: Allocate rx indirection table size dynamically | expand |
On Thu, May 25, 2023 at 11:02:29PM -0700, Shradha Gupta wrote: > Allocate the size of rx indirection table dynamically in netvsc > from the value of size provided by OID_GEN_RECEIVE_SCALE_CAPABILITIES > query instead of using a constant value of ITAB_NUM. > > Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com> > @@ -1548,6 +1549,17 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, > if (ret || rsscap.num_recv_que < 2) > goto out; > > + if (rsscap.num_indirect_tabent && > + rsscap.num_indirect_tabent <= ITAB_NUM_MAX) > + ndc->rx_table_sz = rsscap.num_indirect_tabent; > + else > + ndc->rx_table_sz = ITAB_NUM; > + > + ndc->rx_table = kcalloc(ndc->rx_table_sz, sizeof(u16), > + GFP_KERNEL); nit: the above could fit on a single line. > + if (!ndc->rx_table) I think you need to set ret to an error value here, as err_dev_remv will call return ERR_PTR(ret). > + goto err_dev_remv; > + > /* This guarantees that num_possible_rss_qs <= num_online_cpus */ > num_possible_rss_qs = min_t(u32, num_online_cpus(), > rsscap.num_recv_que); > @@ -1558,7 +1570,7 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, > net_device->num_chn = min(net_device->max_chn, device_info->num_chn); > > if (!netif_is_rxfh_configured(net)) { > - for (i = 0; i < ITAB_NUM; i++) > + for (i = 0; i < ndc->rx_table_sz; i++) > ndc->rx_table[i] = ethtool_rxfh_indir_default( > i, net_device->num_chn); > } > @@ -1596,11 +1608,18 @@ void rndis_filter_device_remove(struct hv_device *dev, > struct netvsc_device *net_dev) > { > struct rndis_device *rndis_dev = net_dev->extension; > + struct net_device *net = hv_get_drvdata(dev); > + struct net_device_context *ndc = netdev_priv(net); nit: Please use reverse xmas tree order - longest line to shortest - for local variable declaration sin networking code. struct rndis_device *rndis_dev = net_dev->extension; struct net_device *net = hv_get_drvdata(dev); struct net_device_context *ndc; ndc = netdev_priv(net); > > /* Halt and release the rndis device */ > rndis_filter_halt_device(net_dev, rndis_dev); > > netvsc_device_remove(dev); > + > + ndc->rx_table_sz = 0; > + kfree(ndc->rx_table); > + ndc->rx_table = NULL; > + > } > > int rndis_filter_open(struct netvsc_device *nvdev)
On 5/26/2023 11:32 AM, Shradha Gupta wrote: > Allocate the size of rx indirection table dynamically in netvsc > from the value of size provided by OID_GEN_RECEIVE_SCALE_CAPABILITIES > query instead of using a constant value of ITAB_NUM. > > Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com> > Tested-on: Ubuntu22 (azure VM, SKU size: Standard_F72s_v2) > Testcases: > 1. ethtool -x eth0 output > 2. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-Synthetic > 3. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-SRIOV > > --- > Changes in v3: > * Changed the data type of rx_table_sz to u32 > * Moved the rx indirection table free to rndis_filter_device_remove() > * Device add will fail with error if not enough memory is available > * Changed kzmalloc to kcalloc as suggested in checkpatch script > * Removed redundant log if memory allocation failed. > --- > drivers/net/hyperv/hyperv_net.h | 5 ++++- > drivers/net/hyperv/netvsc_drv.c | 10 ++++++---- > drivers/net/hyperv/rndis_filter.c | 27 +++++++++++++++++++++++---- > 3 files changed, 33 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h > index dd5919ec408b..c40868f287a9 100644 > --- a/drivers/net/hyperv/hyperv_net.h > +++ b/drivers/net/hyperv/hyperv_net.h > @@ -74,6 +74,7 @@ struct ndis_recv_scale_cap { /* NDIS_RECEIVE_SCALE_CAPABILITIES */ > #define NDIS_RSS_HASH_SECRET_KEY_MAX_SIZE_REVISION_2 40 > > #define ITAB_NUM 128 > +#define ITAB_NUM_MAX 256 > > struct ndis_recv_scale_param { /* NDIS_RECEIVE_SCALE_PARAMETERS */ > struct ndis_obj_header hdr; > @@ -1034,7 +1035,9 @@ struct net_device_context { > > u32 tx_table[VRSS_SEND_TAB_SIZE]; > > - u16 rx_table[ITAB_NUM]; > + u16 *rx_table; > + > + u32 rx_table_sz; > > /* Ethtool settings */ > u8 duplex; > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > index 0103ff914024..3ba3c8fb28a5 100644 > --- a/drivers/net/hyperv/netvsc_drv.c > +++ b/drivers/net/hyperv/netvsc_drv.c > @@ -1747,7 +1747,9 @@ static u32 netvsc_get_rxfh_key_size(struct net_device *dev) > > static u32 netvsc_rss_indir_size(struct net_device *dev) > { > - return ITAB_NUM; > + struct net_device_context *ndc = netdev_priv(dev); > + > + return ndc->rx_table_sz; > } > > static int netvsc_get_rxfh(struct net_device *dev, u32 *indir, u8 *key, > @@ -1766,7 +1768,7 @@ static int netvsc_get_rxfh(struct net_device *dev, u32 *indir, u8 *key, > > rndis_dev = ndev->extension; > if (indir) { > - for (i = 0; i < ITAB_NUM; i++) > + for (i = 0; i < ndc->rx_table_sz; i++) > indir[i] = ndc->rx_table[i]; > } > > @@ -1792,11 +1794,11 @@ static int netvsc_set_rxfh(struct net_device *dev, const u32 *indir, > > rndis_dev = ndev->extension; > if (indir) { > - for (i = 0; i < ITAB_NUM; i++) > + for (i = 0; i < ndc->rx_table_sz; i++) > if (indir[i] >= ndev->num_chn) > return -EINVAL; > > - for (i = 0; i < ITAB_NUM; i++) > + for (i = 0; i < ndc->rx_table_sz; i++) > ndc->rx_table[i] = indir[i]; > } > > diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c > index eea777ec2541..dc7b9b326690 100644 > --- a/drivers/net/hyperv/rndis_filter.c > +++ b/drivers/net/hyperv/rndis_filter.c > @@ -21,6 +21,7 @@ > #include <linux/rtnetlink.h> > #include <linux/ucs2_string.h> > #include <linux/string.h> > +#include <linux/slab.h> > > #include "hyperv_net.h" > #include "netvsc_trace.h" > @@ -927,7 +928,7 @@ static int rndis_set_rss_param_msg(struct rndis_device *rdev, > struct rndis_set_request *set; > struct rndis_set_complete *set_complete; > u32 extlen = sizeof(struct ndis_recv_scale_param) + > - 4 * ITAB_NUM + NETVSC_HASH_KEYLEN; > + 4 * ndc->rx_table_sz + NETVSC_HASH_KEYLEN; > struct ndis_recv_scale_param *rssp; > u32 *itab; > u8 *keyp; > @@ -953,7 +954,7 @@ static int rndis_set_rss_param_msg(struct rndis_device *rdev, > rssp->hashinfo = NDIS_HASH_FUNC_TOEPLITZ | NDIS_HASH_IPV4 | > NDIS_HASH_TCP_IPV4 | NDIS_HASH_IPV6 | > NDIS_HASH_TCP_IPV6; > - rssp->indirect_tabsize = 4*ITAB_NUM; > + rssp->indirect_tabsize = 4 * ndc->rx_table_sz; > rssp->indirect_taboffset = sizeof(struct ndis_recv_scale_param); > rssp->hashkey_size = NETVSC_HASH_KEYLEN; > rssp->hashkey_offset = rssp->indirect_taboffset + > @@ -961,7 +962,7 @@ static int rndis_set_rss_param_msg(struct rndis_device *rdev, > > /* Set indirection table entries */ > itab = (u32 *)(rssp + 1); > - for (i = 0; i < ITAB_NUM; i++) > + for (i = 0; i < ndc->rx_table_sz; i++) > itab[i] = ndc->rx_table[i]; > > /* Set hask key values */ > @@ -1548,6 +1549,17 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, > if (ret || rsscap.num_recv_que < 2) > goto out; > > + if (rsscap.num_indirect_tabent && > + rsscap.num_indirect_tabent <= ITAB_NUM_MAX) > + ndc->rx_table_sz = rsscap.num_indirect_tabent; > + else > + ndc->rx_table_sz = ITAB_NUM; > + > + ndc->rx_table = kcalloc(ndc->rx_table_sz, sizeof(u16), > + GFP_KERNEL); > + if (!ndc->rx_table) > + goto err_dev_remv; > + > /* This guarantees that num_possible_rss_qs <= num_online_cpus */ > num_possible_rss_qs = min_t(u32, num_online_cpus(), > rsscap.num_recv_que); > @@ -1558,7 +1570,7 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, > net_device->num_chn = min(net_device->max_chn, device_info->num_chn); > > if (!netif_is_rxfh_configured(net)) { > - for (i = 0; i < ITAB_NUM; i++) > + for (i = 0; i < ndc->rx_table_sz; i++) > ndc->rx_table[i] = ethtool_rxfh_indir_default( > i, net_device->num_chn); > } > @@ -1596,11 +1608,18 @@ void rndis_filter_device_remove(struct hv_device *dev, > struct netvsc_device *net_dev) > { > struct rndis_device *rndis_dev = net_dev->extension; > + struct net_device *net = hv_get_drvdata(dev); > + struct net_device_context *ndc = netdev_priv(net); > > /* Halt and release the rndis device */ > rndis_filter_halt_device(net_dev, rndis_dev); > > netvsc_device_remove(dev); Shouldn't the netvsc_device_remove be called post table cleanup ? or better, the cleanup should happen as part of netvsc_device_remove operation ? This looks a bug to me as with remove operation, we already cleaned up the device and the association between context and device is removed. > + > + ndc->rx_table_sz = 0; > + kfree(ndc->rx_table); > + ndc->rx_table = NULL; > + > } > > int rndis_filter_open(struct netvsc_device *nvdev)
On Mon, May 29, 2023 at 03:09:49PM +0530, Praveen Kumar wrote: > On 5/26/2023 11:32 AM, Shradha Gupta wrote: > > Allocate the size of rx indirection table dynamically in netvsc > > from the value of size provided by OID_GEN_RECEIVE_SCALE_CAPABILITIES > > query instead of using a constant value of ITAB_NUM. > > > > Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com> > > Tested-on: Ubuntu22 (azure VM, SKU size: Standard_F72s_v2) > > Testcases: > > 1. ethtool -x eth0 output > > 2. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-Synthetic > > 3. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-SRIOV > > > > --- > > Changes in v3: > > * Changed the data type of rx_table_sz to u32 > > * Moved the rx indirection table free to rndis_filter_device_remove() > > * Device add will fail with error if not enough memory is available > > * Changed kzmalloc to kcalloc as suggested in checkpatch script > > * Removed redundant log if memory allocation failed. > > --- > > drivers/net/hyperv/hyperv_net.h | 5 ++++- > > drivers/net/hyperv/netvsc_drv.c | 10 ++++++---- > > drivers/net/hyperv/rndis_filter.c | 27 +++++++++++++++++++++++---- > > 3 files changed, 33 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h > > index dd5919ec408b..c40868f287a9 100644 > > --- a/drivers/net/hyperv/hyperv_net.h > > +++ b/drivers/net/hyperv/hyperv_net.h > > @@ -74,6 +74,7 @@ struct ndis_recv_scale_cap { /* NDIS_RECEIVE_SCALE_CAPABILITIES */ > > #define NDIS_RSS_HASH_SECRET_KEY_MAX_SIZE_REVISION_2 40 > > > > #define ITAB_NUM 128 > > +#define ITAB_NUM_MAX 256 > > > > struct ndis_recv_scale_param { /* NDIS_RECEIVE_SCALE_PARAMETERS */ > > struct ndis_obj_header hdr; > > @@ -1034,7 +1035,9 @@ struct net_device_context { > > > > u32 tx_table[VRSS_SEND_TAB_SIZE]; > > > > - u16 rx_table[ITAB_NUM]; > > + u16 *rx_table; > > + > > + u32 rx_table_sz; > > > > /* Ethtool settings */ > > u8 duplex; > > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > > index 0103ff914024..3ba3c8fb28a5 100644 > > --- a/drivers/net/hyperv/netvsc_drv.c > > +++ b/drivers/net/hyperv/netvsc_drv.c > > @@ -1747,7 +1747,9 @@ static u32 netvsc_get_rxfh_key_size(struct net_device *dev) > > > > static u32 netvsc_rss_indir_size(struct net_device *dev) > > { > > - return ITAB_NUM; > > + struct net_device_context *ndc = netdev_priv(dev); > > + > > + return ndc->rx_table_sz; > > } > > > > static int netvsc_get_rxfh(struct net_device *dev, u32 *indir, u8 *key, > > @@ -1766,7 +1768,7 @@ static int netvsc_get_rxfh(struct net_device *dev, u32 *indir, u8 *key, > > > > rndis_dev = ndev->extension; > > if (indir) { > > - for (i = 0; i < ITAB_NUM; i++) > > + for (i = 0; i < ndc->rx_table_sz; i++) > > indir[i] = ndc->rx_table[i]; > > } > > > > @@ -1792,11 +1794,11 @@ static int netvsc_set_rxfh(struct net_device *dev, const u32 *indir, > > > > rndis_dev = ndev->extension; > > if (indir) { > > - for (i = 0; i < ITAB_NUM; i++) > > + for (i = 0; i < ndc->rx_table_sz; i++) > > if (indir[i] >= ndev->num_chn) > > return -EINVAL; > > > > - for (i = 0; i < ITAB_NUM; i++) > > + for (i = 0; i < ndc->rx_table_sz; i++) > > ndc->rx_table[i] = indir[i]; > > } > > > > diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c > > index eea777ec2541..dc7b9b326690 100644 > > --- a/drivers/net/hyperv/rndis_filter.c > > +++ b/drivers/net/hyperv/rndis_filter.c > > @@ -21,6 +21,7 @@ > > #include <linux/rtnetlink.h> > > #include <linux/ucs2_string.h> > > #include <linux/string.h> > > +#include <linux/slab.h> > > > > #include "hyperv_net.h" > > #include "netvsc_trace.h" > > @@ -927,7 +928,7 @@ static int rndis_set_rss_param_msg(struct rndis_device *rdev, > > struct rndis_set_request *set; > > struct rndis_set_complete *set_complete; > > u32 extlen = sizeof(struct ndis_recv_scale_param) + > > - 4 * ITAB_NUM + NETVSC_HASH_KEYLEN; > > + 4 * ndc->rx_table_sz + NETVSC_HASH_KEYLEN; > > struct ndis_recv_scale_param *rssp; > > u32 *itab; > > u8 *keyp; > > @@ -953,7 +954,7 @@ static int rndis_set_rss_param_msg(struct rndis_device *rdev, > > rssp->hashinfo = NDIS_HASH_FUNC_TOEPLITZ | NDIS_HASH_IPV4 | > > NDIS_HASH_TCP_IPV4 | NDIS_HASH_IPV6 | > > NDIS_HASH_TCP_IPV6; > > - rssp->indirect_tabsize = 4*ITAB_NUM; > > + rssp->indirect_tabsize = 4 * ndc->rx_table_sz; > > rssp->indirect_taboffset = sizeof(struct ndis_recv_scale_param); > > rssp->hashkey_size = NETVSC_HASH_KEYLEN; > > rssp->hashkey_offset = rssp->indirect_taboffset + > > @@ -961,7 +962,7 @@ static int rndis_set_rss_param_msg(struct rndis_device *rdev, > > > > /* Set indirection table entries */ > > itab = (u32 *)(rssp + 1); > > - for (i = 0; i < ITAB_NUM; i++) > > + for (i = 0; i < ndc->rx_table_sz; i++) > > itab[i] = ndc->rx_table[i]; > > > > /* Set hask key values */ > > @@ -1548,6 +1549,17 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, > > if (ret || rsscap.num_recv_que < 2) > > goto out; > > > > + if (rsscap.num_indirect_tabent && > > + rsscap.num_indirect_tabent <= ITAB_NUM_MAX) > > + ndc->rx_table_sz = rsscap.num_indirect_tabent; > > + else > > + ndc->rx_table_sz = ITAB_NUM; > > + > > + ndc->rx_table = kcalloc(ndc->rx_table_sz, sizeof(u16), > > + GFP_KERNEL); > > + if (!ndc->rx_table) > > + goto err_dev_remv; > > + > > /* This guarantees that num_possible_rss_qs <= num_online_cpus */ > > num_possible_rss_qs = min_t(u32, num_online_cpus(), > > rsscap.num_recv_que); > > @@ -1558,7 +1570,7 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, > > net_device->num_chn = min(net_device->max_chn, device_info->num_chn); > > > > if (!netif_is_rxfh_configured(net)) { > > - for (i = 0; i < ITAB_NUM; i++) > > + for (i = 0; i < ndc->rx_table_sz; i++) > > ndc->rx_table[i] = ethtool_rxfh_indir_default( > > i, net_device->num_chn); > > } > > @@ -1596,11 +1608,18 @@ void rndis_filter_device_remove(struct hv_device *dev, > > struct netvsc_device *net_dev) > > { > > struct rndis_device *rndis_dev = net_dev->extension; > > + struct net_device *net = hv_get_drvdata(dev); > > + struct net_device_context *ndc = netdev_priv(net); > > > > /* Halt and release the rndis device */ > > rndis_filter_halt_device(net_dev, rndis_dev); > > > > netvsc_device_remove(dev); > > Shouldn't the netvsc_device_remove be called post table cleanup ? or better, the cleanup should happen as part of netvsc_device_remove operation ? This looks a bug to me as with remove operation, we already cleaned up the device and the association between context and device is removed. The netvsc_device_remove() function is responsible for cleaning up/removing the netvsc_device structures upon events like remove/suspend. The net_device and net_device_context structures(where the rx indirection table exists) remain untouched in netvsc_device_remove(). They(net_device, net_device_context) only get cleaned up in netvsc_remove(). So, the netvsc_device_remove() should not affect the cleanup for the rx indirection table, that we did. > > + > > + ndc->rx_table_sz = 0; > > + kfree(ndc->rx_table); > > + ndc->rx_table = NULL; > > + > > } > > > > int rndis_filter_open(struct netvsc_device *nvdev)
Le 26/05/2023 à 08:02, Shradha Gupta a écrit : > Allocate the size of rx indirection table dynamically in netvsc > from the value of size provided by OID_GEN_RECEIVE_SCALE_CAPABILITIES > query instead of using a constant value of ITAB_NUM. > > Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com> > Tested-on: Ubuntu22 (azure VM, SKU size: Standard_F72s_v2) > Testcases: > 1. ethtool -x eth0 output > 2. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-Synthetic > 3. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-SRIOV > > --- [...] > @@ -1596,11 +1608,18 @@ void rndis_filter_device_remove(struct hv_device *dev, > struct netvsc_device *net_dev) > { > struct rndis_device *rndis_dev = net_dev->extension; > + struct net_device *net = hv_get_drvdata(dev); > + struct net_device_context *ndc = netdev_priv(net); > > /* Halt and release the rndis device */ > rndis_filter_halt_device(net_dev, rndis_dev); > > netvsc_device_remove(dev); > + > + ndc->rx_table_sz = 0; > + kfree(ndc->rx_table); > + ndc->rx_table = NULL; > + Nit: useless empty NL > } > > int rndis_filter_open(struct netvsc_device *nvdev)
Thanks for the comment Christophe. On Mon, May 29, 2023 at 02:49:15PM +0200, Christophe JAILLET wrote: > Le 26/05/2023 ?? 08:02, Shradha Gupta a ??crit??: > >Allocate the size of rx indirection table dynamically in netvsc > >from the value of size provided by OID_GEN_RECEIVE_SCALE_CAPABILITIES > >query instead of using a constant value of ITAB_NUM. > > > >Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com> > >Tested-on: Ubuntu22 (azure VM, SKU size: Standard_F72s_v2) > >Testcases: > >1. ethtool -x eth0 output > >2. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-Synthetic > >3. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-SRIOV > > > >--- > > [...] > > >@@ -1596,11 +1608,18 @@ void rndis_filter_device_remove(struct hv_device *dev, > > struct netvsc_device *net_dev) > > { > > struct rndis_device *rndis_dev = net_dev->extension; > >+ struct net_device *net = hv_get_drvdata(dev); > >+ struct net_device_context *ndc = netdev_priv(net); > > /* Halt and release the rndis device */ > > rndis_filter_halt_device(net_dev, rndis_dev); > > netvsc_device_remove(dev); > >+ > >+ ndc->rx_table_sz = 0; > >+ kfree(ndc->rx_table); > >+ ndc->rx_table = NULL; > >+ > > Nit: useless empty NL This is to prevent any potential double free, or accessing freed memory, etc. As requested by Haiyang in v2 patch > > > } > > int rndis_filter_open(struct netvsc_device *nvdev)
Le 29/05/2023 à 15:30, Shradha Gupta a écrit : > Thanks for the comment Christophe. > On Mon, May 29, 2023 at 02:49:15PM +0200, Christophe JAILLET wrote: >> Le 26/05/2023 ?? 08:02, Shradha Gupta a ??crit??: >>> Allocate the size of rx indirection table dynamically in netvsc >> >from the value of size provided by OID_GEN_RECEIVE_SCALE_CAPABILITIES >>> query instead of using a constant value of ITAB_NUM. >>> >>> Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com> >>> Tested-on: Ubuntu22 (azure VM, SKU size: Standard_F72s_v2) >>> Testcases: >>> 1. ethtool -x eth0 output >>> 2. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-Synthetic >>> 3. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-SRIOV >>> >>> --- >> >> [...] >> >>> @@ -1596,11 +1608,18 @@ void rndis_filter_device_remove(struct hv_device *dev, >>> struct netvsc_device *net_dev) >>> { >>> struct rndis_device *rndis_dev = net_dev->extension; >>> + struct net_device *net = hv_get_drvdata(dev); >>> + struct net_device_context *ndc = netdev_priv(net); >>> /* Halt and release the rndis device */ >>> rndis_filter_halt_device(net_dev, rndis_dev); >>> netvsc_device_remove(dev); >>> + >>> + ndc->rx_table_sz = 0; >>> + kfree(ndc->rx_table); >>> + ndc->rx_table = NULL; >>> + >> >> Nit: useless empty NL > This is to prevent any potential double free, or accessing freed memory, etc. > As requested by Haiyang in v2 patch Setting ndc->rx_table to NULL is fine, but there is a useless *newline* (NL) just after. If you have to send a v4, you can save a line of code. CJ >> >>> } >>> int rndis_filter_open(struct netvsc_device *nvdev) >
> -----Original Message----- > From: Shradha Gupta <shradhagupta@linux.microsoft.com> > Sent: Friday, May 26, 2023 2:02 AM > To: linux-kernel@vger.kernel.org; linux-hyperv@vger.kernel.org; > netdev@vger.kernel.org > Cc: Shradha Gupta <shradhagupta@linux.microsoft.com>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang > <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>; Dexuan Cui > <decui@microsoft.com>; Long Li <longli@microsoft.com>; Michael Kelley > (LINUX) <mikelley@microsoft.com>; David S. Miller <davem@davemloft.net>; > Steen Hegelund <steen.hegelund@microchip.com>; Simon Horman > <simon.horman@corigine.com> > Subject: [PATCH v3] hv_netvsc: Allocate rx indirection table size dynamically > > Allocate the size of rx indirection table dynamically in netvsc > from the value of size provided by OID_GEN_RECEIVE_SCALE_CAPABILITIES > query instead of using a constant value of ITAB_NUM. > > Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com> > Tested-on: Ubuntu22 (azure VM, SKU size: Standard_F72s_v2) > Testcases: > 1. ethtool -x eth0 output > 2. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION- > NTTTCP-Synthetic > 3. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION- > NTTTCP-SRIOV Thank you! Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
On 5/29/2023 4:47 PM, Shradha Gupta wrote: > On Mon, May 29, 2023 at 03:09:49PM +0530, Praveen Kumar wrote: >> On 5/26/2023 11:32 AM, Shradha Gupta wrote: >>> Allocate the size of rx indirection table dynamically in netvsc >>> from the value of size provided by OID_GEN_RECEIVE_SCALE_CAPABILITIES >>> query instead of using a constant value of ITAB_NUM. >>> >>> Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com> >>> Tested-on: Ubuntu22 (azure VM, SKU size: Standard_F72s_v2) >>> Testcases: >>> 1. ethtool -x eth0 output >>> 2. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-Synthetic >>> 3. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-SRIOV >>> >>> --- >>> Changes in v3: >>> * Changed the data type of rx_table_sz to u32 >>> * Moved the rx indirection table free to rndis_filter_device_remove() >>> * Device add will fail with error if not enough memory is available >>> * Changed kzmalloc to kcalloc as suggested in checkpatch script >>> * Removed redundant log if memory allocation failed. >>> --- >>> drivers/net/hyperv/hyperv_net.h | 5 ++++- >>> drivers/net/hyperv/netvsc_drv.c | 10 ++++++---- >>> drivers/net/hyperv/rndis_filter.c | 27 +++++++++++++++++++++++---- >>> 3 files changed, 33 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h >>> index dd5919ec408b..c40868f287a9 100644 >>> --- a/drivers/net/hyperv/hyperv_net.h >>> +++ b/drivers/net/hyperv/hyperv_net.h >>> @@ -74,6 +74,7 @@ struct ndis_recv_scale_cap { /* NDIS_RECEIVE_SCALE_CAPABILITIES */ >>> #define NDIS_RSS_HASH_SECRET_KEY_MAX_SIZE_REVISION_2 40 >>> >>> #define ITAB_NUM 128 >>> +#define ITAB_NUM_MAX 256 >>> >>> struct ndis_recv_scale_param { /* NDIS_RECEIVE_SCALE_PARAMETERS */ >>> struct ndis_obj_header hdr; >>> @@ -1034,7 +1035,9 @@ struct net_device_context { >>> >>> u32 tx_table[VRSS_SEND_TAB_SIZE]; >>> >>> - u16 rx_table[ITAB_NUM]; >>> + u16 *rx_table; >>> + >>> + u32 rx_table_sz; >>> >>> /* Ethtool settings */ >>> u8 duplex; >>> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c >>> index 0103ff914024..3ba3c8fb28a5 100644 >>> --- a/drivers/net/hyperv/netvsc_drv.c >>> +++ b/drivers/net/hyperv/netvsc_drv.c >>> @@ -1747,7 +1747,9 @@ static u32 netvsc_get_rxfh_key_size(struct net_device *dev) >>> >>> static u32 netvsc_rss_indir_size(struct net_device *dev) >>> { >>> - return ITAB_NUM; >>> + struct net_device_context *ndc = netdev_priv(dev); >>> + >>> + return ndc->rx_table_sz; >>> } >>> >>> static int netvsc_get_rxfh(struct net_device *dev, u32 *indir, u8 *key, >>> @@ -1766,7 +1768,7 @@ static int netvsc_get_rxfh(struct net_device *dev, u32 *indir, u8 *key, >>> >>> rndis_dev = ndev->extension; >>> if (indir) { >>> - for (i = 0; i < ITAB_NUM; i++) >>> + for (i = 0; i < ndc->rx_table_sz; i++) >>> indir[i] = ndc->rx_table[i]; >>> } >>> >>> @@ -1792,11 +1794,11 @@ static int netvsc_set_rxfh(struct net_device *dev, const u32 *indir, >>> >>> rndis_dev = ndev->extension; >>> if (indir) { >>> - for (i = 0; i < ITAB_NUM; i++) >>> + for (i = 0; i < ndc->rx_table_sz; i++) >>> if (indir[i] >= ndev->num_chn) >>> return -EINVAL; >>> >>> - for (i = 0; i < ITAB_NUM; i++) >>> + for (i = 0; i < ndc->rx_table_sz; i++) >>> ndc->rx_table[i] = indir[i]; >>> } >>> >>> diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c >>> index eea777ec2541..dc7b9b326690 100644 >>> --- a/drivers/net/hyperv/rndis_filter.c >>> +++ b/drivers/net/hyperv/rndis_filter.c >>> @@ -21,6 +21,7 @@ >>> #include <linux/rtnetlink.h> >>> #include <linux/ucs2_string.h> >>> #include <linux/string.h> >>> +#include <linux/slab.h> >>> >>> #include "hyperv_net.h" >>> #include "netvsc_trace.h" >>> @@ -927,7 +928,7 @@ static int rndis_set_rss_param_msg(struct rndis_device *rdev, >>> struct rndis_set_request *set; >>> struct rndis_set_complete *set_complete; >>> u32 extlen = sizeof(struct ndis_recv_scale_param) + >>> - 4 * ITAB_NUM + NETVSC_HASH_KEYLEN; >>> + 4 * ndc->rx_table_sz + NETVSC_HASH_KEYLEN; >>> struct ndis_recv_scale_param *rssp; >>> u32 *itab; >>> u8 *keyp; >>> @@ -953,7 +954,7 @@ static int rndis_set_rss_param_msg(struct rndis_device *rdev, >>> rssp->hashinfo = NDIS_HASH_FUNC_TOEPLITZ | NDIS_HASH_IPV4 | >>> NDIS_HASH_TCP_IPV4 | NDIS_HASH_IPV6 | >>> NDIS_HASH_TCP_IPV6; >>> - rssp->indirect_tabsize = 4*ITAB_NUM; >>> + rssp->indirect_tabsize = 4 * ndc->rx_table_sz; >>> rssp->indirect_taboffset = sizeof(struct ndis_recv_scale_param); >>> rssp->hashkey_size = NETVSC_HASH_KEYLEN; >>> rssp->hashkey_offset = rssp->indirect_taboffset + >>> @@ -961,7 +962,7 @@ static int rndis_set_rss_param_msg(struct rndis_device *rdev, >>> >>> /* Set indirection table entries */ >>> itab = (u32 *)(rssp + 1); >>> - for (i = 0; i < ITAB_NUM; i++) >>> + for (i = 0; i < ndc->rx_table_sz; i++) >>> itab[i] = ndc->rx_table[i]; >>> >>> /* Set hask key values */ >>> @@ -1548,6 +1549,17 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, >>> if (ret || rsscap.num_recv_que < 2) >>> goto out; >>> >>> + if (rsscap.num_indirect_tabent && >>> + rsscap.num_indirect_tabent <= ITAB_NUM_MAX) >>> + ndc->rx_table_sz = rsscap.num_indirect_tabent; >>> + else >>> + ndc->rx_table_sz = ITAB_NUM; >>> + >>> + ndc->rx_table = kcalloc(ndc->rx_table_sz, sizeof(u16), >>> + GFP_KERNEL); >>> + if (!ndc->rx_table) >>> + goto err_dev_remv; >>> + >>> /* This guarantees that num_possible_rss_qs <= num_online_cpus */ >>> num_possible_rss_qs = min_t(u32, num_online_cpus(), >>> rsscap.num_recv_que); >>> @@ -1558,7 +1570,7 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, >>> net_device->num_chn = min(net_device->max_chn, device_info->num_chn); >>> >>> if (!netif_is_rxfh_configured(net)) { >>> - for (i = 0; i < ITAB_NUM; i++) >>> + for (i = 0; i < ndc->rx_table_sz; i++) >>> ndc->rx_table[i] = ethtool_rxfh_indir_default( >>> i, net_device->num_chn); >>> } >>> @@ -1596,11 +1608,18 @@ void rndis_filter_device_remove(struct hv_device *dev, >>> struct netvsc_device *net_dev) >>> { >>> struct rndis_device *rndis_dev = net_dev->extension; >>> + struct net_device *net = hv_get_drvdata(dev); >>> + struct net_device_context *ndc = netdev_priv(net); >>> >>> /* Halt and release the rndis device */ >>> rndis_filter_halt_device(net_dev, rndis_dev); >>> >>> netvsc_device_remove(dev); >> >> Shouldn't the netvsc_device_remove be called post table cleanup ? or better, the cleanup should happen as part of netvsc_device_remove operation ? This looks a bug to me as with remove operation, we already cleaned up the device and the association between context and device is removed. > > The netvsc_device_remove() function is responsible for cleaning up/removing the netvsc_device structures upon events like remove/suspend. The net_device and net_device_context structures(where the rx indirection table exists) remain untouched in netvsc_device_remove(). They(net_device, net_device_context) only get cleaned up in netvsc_remove(). So, the netvsc_device_remove() should not affect the cleanup for the rx indirection table, that we did. > Thanks. Reviewed-by: Praveen Kumar <kumarpraveen@linux.microsoft.com>
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h index dd5919ec408b..c40868f287a9 100644 --- a/drivers/net/hyperv/hyperv_net.h +++ b/drivers/net/hyperv/hyperv_net.h @@ -74,6 +74,7 @@ struct ndis_recv_scale_cap { /* NDIS_RECEIVE_SCALE_CAPABILITIES */ #define NDIS_RSS_HASH_SECRET_KEY_MAX_SIZE_REVISION_2 40 #define ITAB_NUM 128 +#define ITAB_NUM_MAX 256 struct ndis_recv_scale_param { /* NDIS_RECEIVE_SCALE_PARAMETERS */ struct ndis_obj_header hdr; @@ -1034,7 +1035,9 @@ struct net_device_context { u32 tx_table[VRSS_SEND_TAB_SIZE]; - u16 rx_table[ITAB_NUM]; + u16 *rx_table; + + u32 rx_table_sz; /* Ethtool settings */ u8 duplex; diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 0103ff914024..3ba3c8fb28a5 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -1747,7 +1747,9 @@ static u32 netvsc_get_rxfh_key_size(struct net_device *dev) static u32 netvsc_rss_indir_size(struct net_device *dev) { - return ITAB_NUM; + struct net_device_context *ndc = netdev_priv(dev); + + return ndc->rx_table_sz; } static int netvsc_get_rxfh(struct net_device *dev, u32 *indir, u8 *key, @@ -1766,7 +1768,7 @@ static int netvsc_get_rxfh(struct net_device *dev, u32 *indir, u8 *key, rndis_dev = ndev->extension; if (indir) { - for (i = 0; i < ITAB_NUM; i++) + for (i = 0; i < ndc->rx_table_sz; i++) indir[i] = ndc->rx_table[i]; } @@ -1792,11 +1794,11 @@ static int netvsc_set_rxfh(struct net_device *dev, const u32 *indir, rndis_dev = ndev->extension; if (indir) { - for (i = 0; i < ITAB_NUM; i++) + for (i = 0; i < ndc->rx_table_sz; i++) if (indir[i] >= ndev->num_chn) return -EINVAL; - for (i = 0; i < ITAB_NUM; i++) + for (i = 0; i < ndc->rx_table_sz; i++) ndc->rx_table[i] = indir[i]; } diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c index eea777ec2541..dc7b9b326690 100644 --- a/drivers/net/hyperv/rndis_filter.c +++ b/drivers/net/hyperv/rndis_filter.c @@ -21,6 +21,7 @@ #include <linux/rtnetlink.h> #include <linux/ucs2_string.h> #include <linux/string.h> +#include <linux/slab.h> #include "hyperv_net.h" #include "netvsc_trace.h" @@ -927,7 +928,7 @@ static int rndis_set_rss_param_msg(struct rndis_device *rdev, struct rndis_set_request *set; struct rndis_set_complete *set_complete; u32 extlen = sizeof(struct ndis_recv_scale_param) + - 4 * ITAB_NUM + NETVSC_HASH_KEYLEN; + 4 * ndc->rx_table_sz + NETVSC_HASH_KEYLEN; struct ndis_recv_scale_param *rssp; u32 *itab; u8 *keyp; @@ -953,7 +954,7 @@ static int rndis_set_rss_param_msg(struct rndis_device *rdev, rssp->hashinfo = NDIS_HASH_FUNC_TOEPLITZ | NDIS_HASH_IPV4 | NDIS_HASH_TCP_IPV4 | NDIS_HASH_IPV6 | NDIS_HASH_TCP_IPV6; - rssp->indirect_tabsize = 4*ITAB_NUM; + rssp->indirect_tabsize = 4 * ndc->rx_table_sz; rssp->indirect_taboffset = sizeof(struct ndis_recv_scale_param); rssp->hashkey_size = NETVSC_HASH_KEYLEN; rssp->hashkey_offset = rssp->indirect_taboffset + @@ -961,7 +962,7 @@ static int rndis_set_rss_param_msg(struct rndis_device *rdev, /* Set indirection table entries */ itab = (u32 *)(rssp + 1); - for (i = 0; i < ITAB_NUM; i++) + for (i = 0; i < ndc->rx_table_sz; i++) itab[i] = ndc->rx_table[i]; /* Set hask key values */ @@ -1548,6 +1549,17 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, if (ret || rsscap.num_recv_que < 2) goto out; + if (rsscap.num_indirect_tabent && + rsscap.num_indirect_tabent <= ITAB_NUM_MAX) + ndc->rx_table_sz = rsscap.num_indirect_tabent; + else + ndc->rx_table_sz = ITAB_NUM; + + ndc->rx_table = kcalloc(ndc->rx_table_sz, sizeof(u16), + GFP_KERNEL); + if (!ndc->rx_table) + goto err_dev_remv; + /* This guarantees that num_possible_rss_qs <= num_online_cpus */ num_possible_rss_qs = min_t(u32, num_online_cpus(), rsscap.num_recv_que); @@ -1558,7 +1570,7 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, net_device->num_chn = min(net_device->max_chn, device_info->num_chn); if (!netif_is_rxfh_configured(net)) { - for (i = 0; i < ITAB_NUM; i++) + for (i = 0; i < ndc->rx_table_sz; i++) ndc->rx_table[i] = ethtool_rxfh_indir_default( i, net_device->num_chn); } @@ -1596,11 +1608,18 @@ void rndis_filter_device_remove(struct hv_device *dev, struct netvsc_device *net_dev) { struct rndis_device *rndis_dev = net_dev->extension; + struct net_device *net = hv_get_drvdata(dev); + struct net_device_context *ndc = netdev_priv(net); /* Halt and release the rndis device */ rndis_filter_halt_device(net_dev, rndis_dev); netvsc_device_remove(dev); + + ndc->rx_table_sz = 0; + kfree(ndc->rx_table); + ndc->rx_table = NULL; + } int rndis_filter_open(struct netvsc_device *nvdev)
Allocate the size of rx indirection table dynamically in netvsc from the value of size provided by OID_GEN_RECEIVE_SCALE_CAPABILITIES query instead of using a constant value of ITAB_NUM. Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com> Tested-on: Ubuntu22 (azure VM, SKU size: Standard_F72s_v2) Testcases: 1. ethtool -x eth0 output 2. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-Synthetic 3. LISA testcase:PERF-NETWORK-TCP-THROUGHPUT-MULTICONNECTION-NTTTCP-SRIOV --- Changes in v3: * Changed the data type of rx_table_sz to u32 * Moved the rx indirection table free to rndis_filter_device_remove() * Device add will fail with error if not enough memory is available * Changed kzmalloc to kcalloc as suggested in checkpatch script * Removed redundant log if memory allocation failed. --- drivers/net/hyperv/hyperv_net.h | 5 ++++- drivers/net/hyperv/netvsc_drv.c | 10 ++++++---- drivers/net/hyperv/rndis_filter.c | 27 +++++++++++++++++++++++---- 3 files changed, 33 insertions(+), 9 deletions(-)