Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751169AbaK0Q2z (ORCPT ); Thu, 27 Nov 2014 11:28:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39690 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750887AbaK0Q2x (ORCPT ); Thu, 27 Nov 2014 11:28:53 -0500 Date: Thu, 27 Nov 2014 18:28:04 +0200 From: "Michael S. Tsirkin" To: David Hildenbrand Cc: linux-kernel@vger.kernel.org, David Miller , cornelia.huck@de.ibm.com, rusty@au1.ibm.com, nab@linux-iscsi.org, pbonzini@redhat.com, thuth@linux.vnet.ibm.com, Rusty Russell , Brian Swetland , Christian Borntraeger , Pawel Moll , Ohad Ben-Cohen , Amit Shah , Arnd Bergmann , Greg Kroah-Hartman , linux390@de.ibm.com, Martin Schwidefsky , Heiko Carstens , Ashutosh Dixit , Sudeep Dutt , Siva Yerramreddy , Joel Stanley , virtualization@lists.linux-foundation.org, lguest@lists.ozlabs.org, linux-s390@vger.kernel.org Subject: Re: [PATCH v5 01/45] virtio: use u32, not bitmap for struct virtio_device's features Message-ID: <20141127162804.GB27124@redhat.com> References: <1417091078-24611-1-git-send-email-mst@redhat.com> <1417091078-24611-2-git-send-email-mst@redhat.com> <20141127171542.3dbb2bc2@thinkpad-w530> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20141127171542.3dbb2bc2@thinkpad-w530> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Nov 27, 2014 at 05:15:42PM +0100, David Hildenbrand wrote: > > From: Rusty Russell > > > > It seemed like a good idea, but it's actually a pain when we get more > > than 32 feature bits. Just change it to a u32 for now. > > > > Cc: Brian Swetland > > Cc: Christian Borntraeger > > Signed-off-by: Rusty Russell > > Signed-off-by: Cornelia Huck > > Acked-by: Pawel Moll > > Acked-by: Ohad Ben-Cohen > > > > Signed-off-by: Michael S. Tsirkin > > --- > > Wouldn't it be easier to turn > test_bit(i, dev->features) > > into a simple makro like > #define test_bit(i, features) (features & (1 << i)) > > Similar one for clear_bit and set_bit. I don't really see why it's easier. These wrappers resulted in a ton of code converting from bit numbers to set_bit calls and back. E.g. with plan integer you can do (a|b|c) to get a set with 3 bits. > > if names collide with existing functions, we could simply rename them to: > > set_feature_bit() ... > clear_feature_bit() ... In fact that's why we have BIT macro. vdev->features & BIT_ULL(x) is only longer by 1 character than test_feature_bit(vdev, x), and it's much clearer than a non standard wrapper. vdev->features & (1ULL << x) is longer by 3 chars. I guess this is a suggestion to use BIT / BIT_ULL more. Good point. > > diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c > > index e647947..0acd564 100644 > > --- a/drivers/misc/mic/card/mic_virtio.c > > +++ b/drivers/misc/mic/card/mic_virtio.c > > @@ -101,7 +101,7 @@ static void mic_finalize_features(struct virtio_device *vdev) > > bits = min_t(unsigned, feature_len, > > sizeof(vdev->features)) * 8; > > for (i = 0; i < bits; i++) { > > - if (test_bit(i, vdev->features)) > > + if (vdev->features & BIT(bits)) > > Hm, shouldn't that also be "if (vdev->features & (1 << i))"? Ouch. you are right of course. Will fix. > > iowrite8(ioread8(&out_features[i / 8]) | (1 << (i % 8)), > > &out_features[i / 8]); > > } > > > out_free: > > kfree(features); > > kfree(ccw); > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > > index df598dd..7814b1f 100644 > > --- a/drivers/virtio/virtio.c > > +++ b/drivers/virtio/virtio.c > > @@ -49,9 +49,9 @@ static ssize_t features_show(struct device *_d, > > > > /* We actually represent this as a bitstring, as it could be > > * arbitrary length in future. */ > > - for (i = 0; i < ARRAY_SIZE(dev->features)*BITS_PER_LONG; i++) > > + for (i = 0; i < sizeof(dev->features)*8; i++) > > ... sizeof(dev->features) * 8; ... > > Do we have a define for 8 ? We do: BITS_PER_BYTE. > > len += sprintf(buf+len, "%c", > > - test_bit(i, dev->features) ? '1' : '0'); > > + dev->features & (1ULL << i) ? '1' : '0'); > > len += sprintf(buf+len, "\n"); > > return len; > > } > > @@ -168,18 +168,18 @@ static int virtio_dev_probe(struct device *_d) > > device_features = dev->config->get_features(dev); > > > > [...] > > > @@ -483,7 +483,7 @@ int main(int argc, char *argv[]) > > > > /* Set up host side. */ > > vring_init(&vrh.vring, RINGSIZE, __user_addr_min, ALIGN); > > - vringh_init_user(&vrh, vdev.features[0], RINGSIZE, true, > > + vringh_init_user(&vrh, vdev.features, RINGSIZE, true, > > vrh.vring.desc, vrh.vring.avail, vrh.vring.used); > > > > /* No descriptor to get yet... */ > > @@ -652,13 +652,13 @@ int main(int argc, char *argv[]) > > } > > > > /* Test weird (but legal!) indirect. */ > > - if (vdev.features[0] & (1 << VIRTIO_RING_F_INDIRECT_DESC)) { > > + if (vdev.features & (1 << VIRTIO_RING_F_INDIRECT_DESC)) { > > char *data = __user_addr_max - USER_MEM/4; > > struct vring_desc *d = __user_addr_max - USER_MEM/2; > > struct vring vring; > > > > /* Force creation of direct, which we modify. */ > > - vdev.features[0] &= ~(1 << VIRTIO_RING_F_INDIRECT_DESC); > > + vdev.features &= ~(1 << VIRTIO_RING_F_INDIRECT_DESC); > > That could then be something like > > clear_feature_bit(VIRTIO_RING_F_INDIRECT_DESC, vdev.features); > > (I would prefer such makros over repeating bit actions) That's the whole reason for the patch. I guess you disagree with it, but it's much easier to deal with simple integers. > > vq = vring_new_virtqueue(0, RINGSIZE, ALIGN, &vdev, true, > > __user_addr_min, > > never_notify_host, > > > But on the whole this looks good to me. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/