| Message ID | 1399581693-25050-1-git-send-email-haiyangz@microsoft.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, May 08, 2014 at 01:41:33PM -0700, Haiyang Zhang wrote: > This change ensures the driver can be built successfully without the > CONFIG_SYSFS flag. > MS-TFS: 182270 > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> > Reviewed-by: K. Y. Srinivasan <kys@microsoft.com> > --- > drivers/net/hyperv/hyperv_net.h | 28 ++++++++++++++++++++++++++++ > drivers/net/hyperv/netvsc_drv.c | 13 +++++++------ > drivers/net/hyperv/rndis_filter.c | 4 ++-- > 3 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h > index 4b7df5a..02358cb 100644 > --- a/drivers/net/hyperv/hyperv_net.h > +++ b/drivers/net/hyperv/hyperv_net.h > @@ -87,6 +87,8 @@ struct ndis_recv_scale_cap { /* NDIS_RECEIVE_SCALE_CAPABILITIES */ > #define HASH_KEYLEN NDIS_RSS_HASH_SECRET_KEY_MAX_SIZE_REVISION_2 > extern u8 netvsc_hash_key[]; > > +extern unsigned int netvsc_num_queue; > + > struct ndis_recv_scale_param { /* NDIS_RECEIVE_SCALE_PARAMETERS */ > struct ndis_obj_header hdr; > > @@ -178,6 +180,32 @@ struct rndis_device { > unsigned char hw_mac_adr[ETH_ALEN]; > }; > > +static inline void netvsc_record_rx_queue(struct sk_buff *skb, > + struct hv_netvsc_packet *packet, > + struct net_device *ndev) > +{ > +#ifdef CONFIG_SYSFS > + skb_record_rx_queue(skb, packet->channel-> > + offermsg.offer.sub_channel_index % > + ndev->real_num_rx_queues); > +#endif Really? Do all other wireless drivers have this type of #ifdef needed? > +} > + > +static inline void netvsc_show_num_queue(struct device *dev, > + struct net_device *ndev) > +{ > +#ifdef CONFIG_SYSFS > + dev_info(dev, "real num tx,rx queues:%u, %u\n", > + ndev->real_num_tx_queues, ndev->real_num_rx_queues); > +#endif Do network drivers really spam the kernel log with this type of thing? If you have sysfs, why is this needed in the kernel log? > +} > + > +static inline void netvsc_set_num_queue(unsigned int *nq) > +{ > +#ifdef CONFIG_SYSFS > + *nq = num_online_cpus(); > +#endif > +} I really feel that something is odd here... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Greg KH [mailto:greg@kroah.com] > Sent: Thursday, May 8, 2014 4:04 PM > To: Haiyang Zhang > Cc: davem@davemloft.net; netdev@vger.kernel.org; olaf@aepfle.de; > jasowang@redhat.com; driverdev-devel@linuxdriverproject.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH net-next, v2] Add support for netvsc build without > CONFIG_SYSFS flag > > On Thu, May 08, 2014 at 01:41:33PM -0700, Haiyang Zhang wrote: > > This change ensures the driver can be built successfully without the > > CONFIG_SYSFS flag. > > MS-TFS: 182270 > > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> > > Reviewed-by: K. Y. Srinivasan <kys@microsoft.com> > > --- > > drivers/net/hyperv/hyperv_net.h | 28 ++++++++++++++++++++++++++++ > > drivers/net/hyperv/netvsc_drv.c | 13 +++++++------ > > drivers/net/hyperv/rndis_filter.c | 4 ++-- > > 3 files changed, 37 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/net/hyperv/hyperv_net.h > > b/drivers/net/hyperv/hyperv_net.h index 4b7df5a..02358cb 100644 > > --- a/drivers/net/hyperv/hyperv_net.h > > +++ b/drivers/net/hyperv/hyperv_net.h > > @@ -87,6 +87,8 @@ struct ndis_recv_scale_cap { /* > > NDIS_RECEIVE_SCALE_CAPABILITIES */ #define HASH_KEYLEN > > NDIS_RSS_HASH_SECRET_KEY_MAX_SIZE_REVISION_2 > > extern u8 netvsc_hash_key[]; > > > > +extern unsigned int netvsc_num_queue; > > + > > struct ndis_recv_scale_param { /* NDIS_RECEIVE_SCALE_PARAMETERS */ > > struct ndis_obj_header hdr; > > > > @@ -178,6 +180,32 @@ struct rndis_device { > > unsigned char hw_mac_adr[ETH_ALEN]; > > }; > > > > +static inline void netvsc_record_rx_queue(struct sk_buff *skb, > > + struct hv_netvsc_packet *packet, > > + struct net_device *ndev) > > +{ > > +#ifdef CONFIG_SYSFS > > + skb_record_rx_queue(skb, packet->channel-> > > + offermsg.offer.sub_channel_index % > > + ndev->real_num_rx_queues); > > +#endif > > Really? Do all other wireless drivers have this type of #ifdef needed? > > > +} > > + > > +static inline void netvsc_show_num_queue(struct device *dev, > > + struct net_device *ndev) > > +{ > > +#ifdef CONFIG_SYSFS > > + dev_info(dev, "real num tx,rx queues:%u, %u\n", > > + ndev->real_num_tx_queues, ndev->real_num_rx_queues); > #endif > > Do network drivers really spam the kernel log with this type of thing? > If you have sysfs, why is this needed in the kernel log? > > > +} > > + > > +static inline void netvsc_set_num_queue(unsigned int *nq) { #ifdef > > +CONFIG_SYSFS > > + *nq = num_online_cpus(); > > +#endif > > +} > > I really feel that something is odd here... I looked around the other drivers, and the netif_set_real_num_rx_queues() function. It's already switched to no-op without CONFIG_SYSFS flag. So I will rely on this, and don't have to handle the flag in my code. Also, the dev_info can be removed. Thanks, - Haiyang -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Haiyang Zhang <haiyangz@microsoft.com> Date: Thu, 8 May 2014 13:41:33 -0700 > +static inline void netvsc_record_rx_queue(struct sk_buff *skb, > + struct hv_netvsc_packet *packet, > + struct net_device *ndev) > +{ > +#ifdef CONFIG_SYSFS > + skb_record_rx_queue(skb, packet->channel-> > + offermsg.offer.sub_channel_index % > + ndev->real_num_rx_queues); > +#endif > +} This is still fantastically gross, what is so unique about your driver that it needs hacks like this? No other driver to my knowledge does. Figure out what it is that makes your driver so unique, and try to make it conform to how other drivers handle these features without SYSFS ifdef'ery instead. I'm not applying this patch, sorry. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Thursday, May 8, 2014 4:45 PM > To: Haiyang Zhang > Cc: netdev@vger.kernel.org; KY Srinivasan; olaf@aepfle.de; > jasowang@redhat.com; linux-kernel@vger.kernel.org; driverdev- > devel@linuxdriverproject.org > Subject: Re: [PATCH net-next,v2] Add support for netvsc build without > CONFIG_SYSFS flag > > From: Haiyang Zhang <haiyangz@microsoft.com> > Date: Thu, 8 May 2014 13:41:33 -0700 > > > +static inline void netvsc_record_rx_queue(struct sk_buff *skb, > > + struct hv_netvsc_packet *packet, > > + struct net_device *ndev) > > +{ > > +#ifdef CONFIG_SYSFS > > + skb_record_rx_queue(skb, packet->channel-> > > + offermsg.offer.sub_channel_index % > > + ndev->real_num_rx_queues); > > +#endif > > +} > > This is still fantastically gross, what is so unique about your driver that it needs > hacks like this? No other driver to my knowledge does. > > Figure out what it is that makes your driver so unique, and try to make it > conform to how other drivers handle these features without SYSFS ifdef'ery > instead. > I looked around the other drivers, and the netif_set_real_num_rx_queues() function. It's already switched to no-op without CONFIG_SYSFS flag. So I will rely on this, and don't have to handle the flag in my code. Thanks, - Haiyang -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-05-08 at 20:50 +0000, Haiyang Zhang wrote: > > > -----Original Message----- > > From: David Miller [mailto:davem@davemloft.net] > > Sent: Thursday, May 8, 2014 4:45 PM > > To: Haiyang Zhang > > Cc: netdev@vger.kernel.org; KY Srinivasan; olaf@aepfle.de; > > jasowang@redhat.com; linux-kernel@vger.kernel.org; driverdev- > > devel@linuxdriverproject.org > > Subject: Re: [PATCH net-next,v2] Add support for netvsc build without > > CONFIG_SYSFS flag > > > > From: Haiyang Zhang <haiyangz@microsoft.com> > > Date: Thu, 8 May 2014 13:41:33 -0700 > > > > > +static inline void netvsc_record_rx_queue(struct sk_buff *skb, > > > + struct hv_netvsc_packet *packet, > > > + struct net_device *ndev) > > > +{ > > > +#ifdef CONFIG_SYSFS > > > + skb_record_rx_queue(skb, packet->channel-> > > > + offermsg.offer.sub_channel_index % > > > + ndev->real_num_rx_queues); > > > +#endif > > > +} > > > > This is still fantastically gross, what is so unique about your driver that it needs > > hacks like this? No other driver to my knowledge does. > > > > Figure out what it is that makes your driver so unique, and try to make it > > conform to how other drivers handle these features without SYSFS ifdef'ery > > instead. > > > > I looked around the other drivers, and the netif_set_real_num_rx_queues() function. > It's already switched to no-op without CONFIG_SYSFS flag. So I will rely on this, and > don't have to handle the flag in my code. I think most other drivers have a 1-1 mapping between hardware RX queues and the RX queue indices reported to Linux. It appears that in this case sub_channel_index is the 'hardware' queue number, but you think there is not a 1-1 mapping. Why is that? Ben.
> ________________________________________ > From: Ben Hutchings <ben@decadent.org.uk> > Sent: Sunday, May 11, 2014 9:07 AM > To: Haiyang Zhang > I think most other drivers have a 1-1 mapping between hardware RX queues > and the RX queue indices reported to Linux. It appears that in this > case sub_channel_index is the 'hardware' queue number, but you think > there is not a 1-1 mapping. Why is that? Actually, it is 1-1 mapping. The number of channels equals to the number of queues. In my updated patch (v3) the unnecessary "% ndev->real_num_rx_queues" has been removed. Thanks, - Haiyang-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h index 4b7df5a..02358cb 100644 --- a/drivers/net/hyperv/hyperv_net.h +++ b/drivers/net/hyperv/hyperv_net.h @@ -87,6 +87,8 @@ struct ndis_recv_scale_cap { /* NDIS_RECEIVE_SCALE_CAPABILITIES */ #define HASH_KEYLEN NDIS_RSS_HASH_SECRET_KEY_MAX_SIZE_REVISION_2 extern u8 netvsc_hash_key[]; +extern unsigned int netvsc_num_queue; + struct ndis_recv_scale_param { /* NDIS_RECEIVE_SCALE_PARAMETERS */ struct ndis_obj_header hdr; @@ -178,6 +180,32 @@ struct rndis_device { unsigned char hw_mac_adr[ETH_ALEN]; }; +static inline void netvsc_record_rx_queue(struct sk_buff *skb, + struct hv_netvsc_packet *packet, + struct net_device *ndev) +{ +#ifdef CONFIG_SYSFS + skb_record_rx_queue(skb, packet->channel-> + offermsg.offer.sub_channel_index % + ndev->real_num_rx_queues); +#endif +} + +static inline void netvsc_show_num_queue(struct device *dev, + struct net_device *ndev) +{ +#ifdef CONFIG_SYSFS + dev_info(dev, "real num tx,rx queues:%u, %u\n", + ndev->real_num_tx_queues, ndev->real_num_rx_queues); +#endif +} + +static inline void netvsc_set_num_queue(unsigned int *nq) +{ +#ifdef CONFIG_SYSFS + *nq = num_online_cpus(); +#endif +} /* Interface */ int netvsc_device_add(struct hv_device *device, void *additional_info); diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 939e3af..11794a0 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -52,6 +52,8 @@ static int ring_size = 128; module_param(ring_size, int, S_IRUGO); MODULE_PARM_DESC(ring_size, "Ring buffer size (# of pages)"); +unsigned int netvsc_num_queue = 1; + static void do_set_multicast(struct work_struct *w) { struct net_device_context *ndevctx = @@ -639,9 +641,7 @@ int netvsc_recv_callback(struct hv_device *device_obj, __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), packet->vlan_tci); - skb_record_rx_queue(skb, packet->channel-> - offermsg.offer.sub_channel_index % - net->real_num_rx_queues); + netvsc_record_rx_queue(skb, packet, net); net->stats.rx_packets++; net->stats.rx_bytes += packet->total_data_buflen; @@ -788,7 +788,7 @@ static int netvsc_probe(struct hv_device *dev, int ret; net = alloc_etherdev_mq(sizeof(struct net_device_context), - num_online_cpus()); + netvsc_num_queue); if (!net) return -ENOMEM; @@ -824,8 +824,7 @@ static int netvsc_probe(struct hv_device *dev, nvdev = hv_get_drvdata(dev); netif_set_real_num_tx_queues(net, nvdev->num_chn); netif_set_real_num_rx_queues(net, nvdev->num_chn); - dev_info(&dev->device, "real num tx,rx queues:%u, %u\n", - net->real_num_tx_queues, net->real_num_rx_queues); + netvsc_show_num_queue(&dev->device, net); ret = register_netdev(net); if (ret != 0) { @@ -897,6 +896,8 @@ static void __exit netvsc_drv_exit(void) static int __init netvsc_drv_init(void) { + netvsc_set_num_queue(&netvsc_num_queue); + if (ring_size < RING_SIZE_MIN) { ring_size = RING_SIZE_MIN; pr_info("Increased ring_size to %d (min allowed)\n", diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c index 99c527a..9db163c 100644 --- a/drivers/net/hyperv/rndis_filter.c +++ b/drivers/net/hyperv/rndis_filter.c @@ -1094,8 +1094,8 @@ int rndis_filter_device_add(struct hv_device *dev, if (ret || rsscap.num_recv_que < 2) goto out; - net_device->num_chn = (num_online_cpus() < rsscap.num_recv_que) ? - num_online_cpus() : rsscap.num_recv_que; + net_device->num_chn = (netvsc_num_queue < rsscap.num_recv_que) ? + netvsc_num_queue : rsscap.num_recv_que; if (net_device->num_chn == 1) goto out;