[PATCH net-next v2 0/3] virtio: style cleanups
From: Stephen Hemminger <stephen@networkplumber.org>
Date: 2016-07-26 16:37:28
Minor things found while doing some inspection code review.
Revision v2 of 2 in this series.
6 messages, 2 authors, 2016-07-26 · open the first message on its own page
From: Stephen Hemminger <stephen@networkplumber.org>
Date: 2016-07-26 16:37:28
Minor things found while doing some inspection code review.
From: Stephen Hemminger <stephen@networkplumber.org>
Date: 2016-07-26 16:37:30
Adjust whitespace to match current favored style. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
--- a/drivers/net/virtio_net.c 2016-07-20 08:12:24.035656119 -0700
+++ b/drivers/net/virtio_net.c 2016-07-26 09:11:06.988788668 -0700@@ -201,7 +201,9 @@ static void give_pages(struct receive_qu struct page *end; /* Find end of list, sew whole thing into vi->rq.pages. */ - for (end = page; end->private; end = (struct page *)end->private); + for (end = page; end->private; end = (struct page *)end->private) + continue; + end->private = (unsigned long)rq->pages; rq->pages = page; }
@@ -233,13 +235,13 @@ static void skb_xmit_done(struct virtque static unsigned int mergeable_ctx_to_buf_truesize(unsigned long mrg_ctx) { unsigned int truesize = mrg_ctx & (MERGEABLE_BUFFER_ALIGN - 1); + return (truesize + 1) * MERGEABLE_BUFFER_ALIGN; } static void *mergeable_ctx_to_buf_address(unsigned long mrg_ctx) { return (void *)(mrg_ctx & -MERGEABLE_BUFFER_ALIGN); - } static unsigned long mergeable_buf_to_ctx(void *buf, unsigned int truesize)
@@ -310,6 +312,7 @@ static struct sk_buff *page_to_skb(struc BUG_ON(offset >= PAGE_SIZE); while (len) { unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len); + skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset, frag_size, truesize); len -= frag_size;
@@ -323,9 +326,10 @@ static struct sk_buff *page_to_skb(struc return skb; } -static struct sk_buff *receive_small(struct virtnet_info *vi, void *buf, unsigned int len) +static struct sk_buff *receive_small(struct virtnet_info *vi, + void *buf, unsigned int len) { - struct sk_buff * skb = buf; + struct sk_buff *skb = buf; len -= vi->hdr_len; skb_trim(skb, len);
@@ -455,6 +459,7 @@ static void receive_buf(struct virtnet_i if (vi->mergeable_rx_bufs) { unsigned long ctx = (unsigned long)buf; void *base = mergeable_ctx_to_buf_address(ctx); + put_page(virt_to_head_page(base)); } else if (vi->big_packets) { give_pages(rq, buf);
@@ -699,7 +704,7 @@ static void refill_work(struct work_stru * we will *never* try to fill again. */ if (still_empty) - schedule_delayed_work(&vi->refill, HZ/2); + schedule_delayed_work(&vi->refill, HZ / 2); } }
@@ -901,12 +906,12 @@ static netdev_tx_t start_xmit(struct sk_ * Since most packets only take 1 or 2 ring slots, stopping the queue * early means 16 slots are typically wasted. */ - if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { + if (sq->vq->num_free < 2 + MAX_SKB_FRAGS) { netif_stop_subqueue(dev, qnum); if (unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { /* More just got used, free them then recheck. */ free_old_xmit_skbs(sq); - if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { + if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) { netif_start_subqueue(dev, qnum); virtqueue_disable_cb(sq->vq); }
@@ -1238,11 +1243,11 @@ static void virtnet_set_affinity(struct } static int virtnet_cpu_callback(struct notifier_block *nfb, - unsigned long action, void *hcpu) + unsigned long action, void *hcpu) { struct virtnet_info *vi = container_of(nfb, struct virtnet_info, nb); - switch(action & ~CPU_TASKS_FROZEN) { + switch (action & ~CPU_TASKS_FROZEN) { case CPU_ONLINE: case CPU_DOWN_FAILED: case CPU_DEAD:
@@ -1259,7 +1264,7 @@ static int virtnet_cpu_callback(struct n } static void virtnet_get_ringparam(struct net_device *dev, - struct ethtool_ringparam *ring) + struct ethtool_ringparam *ring) { struct virtnet_info *vi = netdev_priv(dev);
@@ -1269,7 +1274,6 @@ static void virtnet_get_ringparam(struct ring->tx_pending = ring->tx_max_pending; } - static void virtnet_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info) {
@@ -1279,7 +1283,6 @@ static void virtnet_get_drvinfo(struct n strlcpy(info->driver, KBUILD_MODNAME, sizeof(info->driver)); strlcpy(info->version, VIRTNET_DRIVER_VERSION, sizeof(info->version)); strlcpy(info->bus_info, virtio_bus_name(vdev), sizeof(info->bus_info)); - } /* TODO: Eliminate OOO packets during switching */
@@ -1402,9 +1405,9 @@ static int virtnet_change_mtu(struct net } static const struct net_device_ops virtnet_netdev = { - .ndo_open = virtnet_open, - .ndo_stop = virtnet_close, - .ndo_start_xmit = start_xmit, + .ndo_open = virtnet_open, + .ndo_stop = virtnet_close, + .ndo_start_xmit = start_xmit, .ndo_validate_addr = eth_validate_addr, .ndo_set_mac_address = virtnet_set_mac_address, .ndo_set_rx_mode = virtnet_set_rx_mode,
@@ -1485,6 +1488,7 @@ static void free_receive_bufs(struct vir static void free_receive_page_frags(struct virtnet_info *vi) { int i; + for (i = 0; i < vi->max_queue_pairs; i++) if (vi->rq[i].alloc_frag.page) put_page(vi->rq[i].alloc_frag.page);
@@ -1497,6 +1501,7 @@ static void free_unused_bufs(struct virt for (i = 0; i < vi->max_queue_pairs; i++) { struct virtqueue *vq = vi->sq[i].vq; + while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) dev_kfree_skb(buf); }
@@ -1508,6 +1513,7 @@ static void free_unused_bufs(struct virt if (vi->mergeable_rx_bufs) { unsigned long ctx = (unsigned long)buf; void *base = mergeable_ctx_to_buf_address(ctx); + put_page(virt_to_head_page(base)); } else if (vi->big_packets) { give_pages(&vi->rq[i], buf);
@@ -1660,7 +1666,8 @@ err: #ifdef CONFIG_SYSFS static ssize_t mergeable_rx_buffer_size_show(struct netdev_rx_queue *queue, - struct rx_queue_attribute *attribute, char *buf) + struct rx_queue_attribute *attribute, + char *buf) { struct virtnet_info *vi = netdev_priv(queue->dev); unsigned int queue_index = get_netdev_rx_queue_index(queue);
@@ -1784,7 +1791,8 @@ static int virtnet_probe(struct virtio_d dev->features |= NETIF_F_GSO_ROBUST; if (gso) - dev->features |= dev->hw_features & (NETIF_F_ALL_TSO|NETIF_F_UFO); + dev->features |= dev->hw_features + & (NETIF_F_ALL_TSO | NETIF_F_UFO); /* (!csum && gso) case will be fixed by register_netdev() */ } if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
@@ -1811,8 +1819,9 @@ static int virtnet_probe(struct virtio_d goto free; for_each_possible_cpu(i) { - struct virtnet_stats *virtnet_stats; - virtnet_stats = per_cpu_ptr(vi->stats, i); + struct virtnet_stats *virtnet_stats + = per_cpu_ptr(vi->stats, i); + u64_stats_init(&virtnet_stats->tx_syncp); u64_stats_init(&virtnet_stats->rx_syncp); }
From: Stephen Hemminger <stephen@networkplumber.org>
Date: 2016-07-26 16:37:31
Although sizeof is an operator in C. Preferred usage is to use it as a function. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
--- a/drivers/net/virtio_net.c 2016-07-26 09:13:16.173384238 -0700
+++ b/drivers/net/virtio_net.c 2016-07-26 09:13:16.169384220 -0700@@ -272,7 +272,7 @@ static struct sk_buff *page_to_skb(struc hdr_len = vi->hdr_len; if (vi->mergeable_rx_bufs) - hdr_padded_len = sizeof *hdr; + hdr_padded_len = sizeof(struct virtio_net_hdr_mrg_rxbuf); else hdr_padded_len = sizeof(struct padded_vnet_hdr);
From: Stephen Hemminger <stephen@networkplumber.org>
Date: 2016-07-26 16:37:32
Preferred style is to use kcalloc and kmalloc_array. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
--- a/drivers/net/virtio_net.c 2016-07-26 09:28:21.401557546 -0700
+++ b/drivers/net/virtio_net.c 2016-07-26 09:36:05.107695337 -0700@@ -1551,13 +1551,13 @@ static int virtnet_find_vqs(struct virtn virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ); /* Allocate space for find_vqs parameters */ - vqs = kzalloc(total_vqs * sizeof(*vqs), GFP_KERNEL); + vqs = kcalloc(total_vqs, sizeof(*vqs), GFP_KERNEL); if (!vqs) goto err_vq; - callbacks = kmalloc(total_vqs * sizeof(*callbacks), GFP_KERNEL); + callbacks = kmalloc_array(total_vqs, sizeof(*callbacks), GFP_KERNEL); if (!callbacks) goto err_callback; - names = kmalloc(total_vqs * sizeof(*names), GFP_KERNEL); + names = kmalloc_array(total_vqs, sizeof(*names), GFP_KERNEL); if (!names) goto err_names;
@@ -1613,10 +1613,10 @@ static int virtnet_alloc_queues(struct v { int i; - vi->sq = kzalloc(sizeof(*vi->sq) * vi->max_queue_pairs, GFP_KERNEL); + vi->sq = kcalloc(vi->max_queue_pairs, sizeof(*vi->sq), GFP_KERNEL); if (!vi->sq) goto err_sq; - vi->rq = kzalloc(sizeof(*vi->rq) * vi->max_queue_pairs, GFP_KERNEL); + vi->rq = kcalloc(vi->max_queue_pairs, sizeof(*vi->rq), GFP_KERNEL); if (!vi->rq) goto err_rq;
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2016-07-26 16:58:24
On Tue, Jul 26, 2016 at 09:37:13AM -0700, Stephen Hemminger wrote:
Adjust whitespace to match current favored style. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>--- a/drivers/net/virtio_net.c 2016-07-20 08:12:24.035656119 -0700 +++ b/drivers/net/virtio_net.c 2016-07-26 09:11:06.988788668 -0700@@ -201,7 +201,9 @@ static void give_pages(struct receive_qu struct page *end; /* Find end of list, sew whole thing into vi->rq.pages. */ - for (end = page; end->private; end = (struct page *)end->private); + for (end = page; end->private; end = (struct page *)end->private) + continue; + end->private = (unsigned long)rq->pages; rq->pages = page; }@@ -233,13 +235,13 @@ static void skb_xmit_done(struct virtque static unsigned int mergeable_ctx_to_buf_truesize(unsigned long mrg_ctx) { unsigned int truesize = mrg_ctx & (MERGEABLE_BUFFER_ALIGN - 1); + return (truesize + 1) * MERGEABLE_BUFFER_ALIGN; } static void *mergeable_ctx_to_buf_address(unsigned long mrg_ctx) { return (void *)(mrg_ctx & -MERGEABLE_BUFFER_ALIGN); - } static unsigned long mergeable_buf_to_ctx(void *buf, unsigned int truesize)@@ -310,6 +312,7 @@ static struct sk_buff *page_to_skb(struc BUG_ON(offset >= PAGE_SIZE); while (len) { unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len); + skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset, frag_size, truesize); len -= frag_size;@@ -323,9 +326,10 @@ static struct sk_buff *page_to_skb(struc return skb; } -static struct sk_buff *receive_small(struct virtnet_info *vi, void *buf, unsigned int len) +static struct sk_buff *receive_small(struct virtnet_info *vi, + void *buf, unsigned int len) { - struct sk_buff * skb = buf; + struct sk_buff *skb = buf; len -= vi->hdr_len; skb_trim(skb, len);@@ -455,6 +459,7 @@ static void receive_buf(struct virtnet_i if (vi->mergeable_rx_bufs) { unsigned long ctx = (unsigned long)buf; void *base = mergeable_ctx_to_buf_address(ctx); + put_page(virt_to_head_page(base)); } else if (vi->big_packets) { give_pages(rq, buf);@@ -699,7 +704,7 @@ static void refill_work(struct work_stru * we will *never* try to fill again. */ if (still_empty) - schedule_delayed_work(&vi->refill, HZ/2); + schedule_delayed_work(&vi->refill, HZ / 2); } }@@ -901,12 +906,12 @@ static netdev_tx_t start_xmit(struct sk_ * Since most packets only take 1 or 2 ring slots, stopping the queue * early means 16 slots are typically wasted. */ - if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { + if (sq->vq->num_free < 2 + MAX_SKB_FRAGS) { netif_stop_subqueue(dev, qnum); if (unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { /* More just got used, free them then recheck. */ free_old_xmit_skbs(sq); - if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { + if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) { netif_start_subqueue(dev, qnum); virtqueue_disable_cb(sq->vq); }@@ -1238,11 +1243,11 @@ static void virtnet_set_affinity(struct } static int virtnet_cpu_callback(struct notifier_block *nfb, - unsigned long action, void *hcpu) + unsigned long action, void *hcpu) { struct virtnet_info *vi = container_of(nfb, struct virtnet_info, nb); - switch(action & ~CPU_TASKS_FROZEN) { + switch (action & ~CPU_TASKS_FROZEN) { case CPU_ONLINE: case CPU_DOWN_FAILED: case CPU_DEAD:@@ -1259,7 +1264,7 @@ static int virtnet_cpu_callback(struct n } static void virtnet_get_ringparam(struct net_device *dev, - struct ethtool_ringparam *ring) + struct ethtool_ringparam *ring) { struct virtnet_info *vi = netdev_priv(dev);@@ -1269,7 +1274,6 @@ static void virtnet_get_ringparam(struct ring->tx_pending = ring->tx_max_pending; } - static void virtnet_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info) {@@ -1279,7 +1283,6 @@ static void virtnet_get_drvinfo(struct n strlcpy(info->driver, KBUILD_MODNAME, sizeof(info->driver)); strlcpy(info->version, VIRTNET_DRIVER_VERSION, sizeof(info->version)); strlcpy(info->bus_info, virtio_bus_name(vdev), sizeof(info->bus_info)); - } /* TODO: Eliminate OOO packets during switching */@@ -1402,9 +1405,9 @@ static int virtnet_change_mtu(struct net } static const struct net_device_ops virtnet_netdev = { - .ndo_open = virtnet_open, - .ndo_stop = virtnet_close, - .ndo_start_xmit = start_xmit, + .ndo_open = virtnet_open, + .ndo_stop = virtnet_close, + .ndo_start_xmit = start_xmit, .ndo_validate_addr = eth_validate_addr, .ndo_set_mac_address = virtnet_set_mac_address, .ndo_set_rx_mode = virtnet_set_rx_mode,@@ -1485,6 +1488,7 @@ static void free_receive_bufs(struct vir static void free_receive_page_frags(struct virtnet_info *vi) { int i; + for (i = 0; i < vi->max_queue_pairs; i++) if (vi->rq[i].alloc_frag.page) put_page(vi->rq[i].alloc_frag.page);@@ -1497,6 +1501,7 @@ static void free_unused_bufs(struct virt for (i = 0; i < vi->max_queue_pairs; i++) { struct virtqueue *vq = vi->sq[i].vq; + while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) dev_kfree_skb(buf); }@@ -1508,6 +1513,7 @@ static void free_unused_bufs(struct virt if (vi->mergeable_rx_bufs) { unsigned long ctx = (unsigned long)buf; void *base = mergeable_ctx_to_buf_address(ctx); + put_page(virt_to_head_page(base)); } else if (vi->big_packets) { give_pages(&vi->rq[i], buf);@@ -1660,7 +1666,8 @@ err: #ifdef CONFIG_SYSFS static ssize_t mergeable_rx_buffer_size_show(struct netdev_rx_queue *queue, - struct rx_queue_attribute *attribute, char *buf) + struct rx_queue_attribute *attribute, + char *buf) { struct virtnet_info *vi = netdev_priv(queue->dev); unsigned int queue_index = get_netdev_rx_queue_index(queue);@@ -1784,7 +1791,8 @@ static int virtnet_probe(struct virtio_d dev->features |= NETIF_F_GSO_ROBUST; if (gso) - dev->features |= dev->hw_features & (NETIF_F_ALL_TSO|NETIF_F_UFO); + dev->features |= dev->hw_features + & (NETIF_F_ALL_TSO | NETIF_F_UFO);
I'm sorry, I don't like continuations to start with an operator on the next line. I prefer & on the previous line instead. Is this one a preferred style now? Any pointers?
/* (!csum && gso) case will be fixed by register_netdev() */ } if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))@@ -1811,8 +1819,9 @@ static int virtnet_probe(struct virtio_d goto free; for_each_possible_cpu(i) { - struct virtnet_stats *virtnet_stats; - virtnet_stats = per_cpu_ptr(vi->stats, i); + struct virtnet_stats *virtnet_stats + = per_cpu_ptr(vi->stats, i); +
Same here for =.
u64_stats_init(&virtnet_stats->tx_syncp); u64_stats_init(&virtnet_stats->rx_syncp); }
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2016-07-26 18:11:51
On Tue, Jul 26, 2016 at 09:37:12AM -0700, Stephen Hemminger wrote:
Minor things found while doing some inspection code review.
Acked-by: Michael S. Tsirkin <mst@redhat.com> I guess I'll just queue it up for this release. -- MST