On Sat, Feb 29, 2020 at 2:28 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
>
> Now that I'm playing with patch 14, I took more interest in this code.
>
> On Thu, 27 Feb 2020 16:19:59 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> wrote:
>
> > +
> > +#define PROTO_MASK_SIZE (sizeof(char))
>
> Hmm, this is the size in bytes of the mask, not bits. You may need both.
>
> #define PROTO_MASK_BITS (PROTO_MASK_SIZE * 8)
>
> Because we can have up to 8 protocols per mask size (8 bits in a byte).
>
> > +/**
> > + * tracecmd_tsync_proto_select - Select time sync protocol, to be used for
> > + * timestamp synchronization with a peer
> > + *
> > + * @proto_mask: bitmask array of time sync protocols, supported by the peer
> > + * @length: size of the @protos array
> > + *
> > + * Retuns Id of a time sync protocol, that can be used with the peer, or 0
> > + * in case there is no match with supported protocols
> > + */
> > +unsigned int tracecmd_tsync_proto_select(char *proto_mask, int length)
> > +{
> > + struct tsync_proto *selected = NULL;
> > + struct tsync_proto *proto;
> > + int word;
> > + int id;
> > +
> > + for (word = 0; word < length; word++) {
> > + for (proto = tsync_proto_list; proto; proto = proto->next) {
> > + if (proto->proto_id < word * PROTO_MASK_SIZE)
> > + continue;
>
> The above should be: proto->proto_id < word * PROTO_MASK_BITS
>
> Because what you have currently is:
>
> proto->proto_id < word * 1
>
>
> > +
> > + id = proto->proto_id - word * PROTO_MASK_SIZE;
>
> And here you want PROTO_MASK_BITS, otherwise if we have a proto_id of 2
> (which would fit as a bit in a char), this would become:
>
> id = 2 - 0 * 1 = 1
>
> > + if (id >= PROTO_MASK_SIZE)
>
> Then this is: 2 >= 1 which would skip it.
>
> Hmm, maybe you don't even need PROTO_MASK_SIZE and only need
> PROTO_MASK_BITS.
>
The PROTO_MASK_SIZE is the size of 1 word of the mask array, in bytes.
In the first implementation the array was of integers, and the define
was 4. It was very easy to switch to char, that's why I prefer to keep
the implementation as is - even though that "proto->proto_id < word *
1".
The confusion is the name of the variable "id", which is actually the
index of the bit inside the given bitmask's word. In the example of
proto_id 2:
- the word is 0, this proto_id is in the first word from the bitmask array.
- the index of the bit in this word is 2 ( id = 2 - 0 * 1 = 2 )
- then we can check if the bit, representing the proto_id 2 is up:
if ((1 << id) & proto_mask[word]) ...
> -- Steve
>
> > + continue;
> > +
> > + if ((1 << id) & proto_mask[word]) {
> > + if (selected) {
> > + if (selected->weight < proto->weight)
> > + selected = proto;
> > + } else
> > + selected = proto;
> > + }
> > + }
> > + }
> > +
> > + if (selected)
> > + return selected->proto_id;
> > +
> > + return 0;
> > +}
> > +
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
|