On Wed, Jul 10, 2019 at 02:59:43PM +0200, Jiri Pirko wrote:

Wed, Jul 10, 2019 at 02:38:03PM CEST, mkubecek@suse.cz wrote:

quoted

On Tue, Jul 09, 2019 at 04:18:17PM +0200, Jiri Pirko wrote:

quoted

I understand. So how about avoid the bitfield all together and just
have array of either bits of strings or combinations?

ETHTOOL_CMD_SETTINGS_SET (U->K)
    ETHTOOL_A_HEADER
        ETHTOOL_A_DEV_NAME = "eth3"
    ETHTOOL_A_SETTINGS_PRIV_FLAGS
       ETHTOOL_A_SETTINGS_PRIV_FLAG
           ETHTOOL_A_FLAG_NAME = "legacy-rx"
	   ETHTOOL_A_FLAG_VALUE   (NLA_FLAG)

or the same with index instead of string

ETHTOOL_CMD_SETTINGS_SET (U->K)
    ETHTOOL_A_HEADER
        ETHTOOL_A_DEV_NAME = "eth3"
    ETHTOOL_A_SETTINGS_PRIV_FLAGS
        ETHTOOL_A_SETTINGS_PRIV_FLAG
            ETHTOOL_A_FLAG_INDEX = 0
 	    ETHTOOL_A_FLAG_VALUE   (NLA_FLAG)


For set you can combine both when you want to set multiple bits:

ETHTOOL_CMD_SETTINGS_SET (U->K)
    ETHTOOL_A_HEADER
        ETHTOOL_A_DEV_NAME = "eth3"
    ETHTOOL_A_SETTINGS_PRIV_FLAGS
        ETHTOOL_A_SETTINGS_PRIV_FLAG
            ETHTOOL_A_FLAG_INDEX = 2
 	    ETHTOOL_A_FLAG_VALUE   (NLA_FLAG)
        ETHTOOL_A_SETTINGS_PRIV_FLAG
            ETHTOOL_A_FLAG_INDEX = 8
 	    ETHTOOL_A_FLAG_VALUE   (NLA_FLAG)
        ETHTOOL_A_SETTINGS_PRIV_FLAG
            ETHTOOL_A_FLAG_NAME = "legacy-rx"
 	    ETHTOOL_A_FLAG_VALUE   (NLA_FLAG)


For get this might be a bit bigger message:

ETHTOOL_CMD_SETTINGS_GET_REPLY (K->U)
    ETHTOOL_A_HEADER
        ETHTOOL_A_DEV_NAME = "eth3"
    ETHTOOL_A_SETTINGS_PRIV_FLAGS
        ETHTOOL_A_SETTINGS_PRIV_FLAG
            ETHTOOL_A_FLAG_INDEX = 0
            ETHTOOL_A_FLAG_NAME = "legacy-rx"
 	    ETHTOOL_A_FLAG_VALUE   (NLA_FLAG)
        ETHTOOL_A_SETTINGS_PRIV_FLAG
            ETHTOOL_A_FLAG_INDEX = 1
            ETHTOOL_A_FLAG_NAME = "vf-ipsec"
 	    ETHTOOL_A_FLAG_VALUE   (NLA_FLAG)
        ETHTOOL_A_SETTINGS_PRIV_FLAG
            ETHTOOL_A_FLAG_INDEX = 8
            ETHTOOL_A_FLAG_NAME = "something-else"
 	    ETHTOOL_A_FLAG_VALUE   (NLA_FLAG)

This is perfect for "one shot" applications but not so much for long
running ones, either "ethtool --monitor" or management or monitoring
daemons. Repeating the names in every notification message would be
a waste, it's much more convenient to load the strings only once and

Yeah, for those aplications, the ETHTOOL_A_FLAG_NAME could be omitted

quoted

cache them. Even if we omit the names in notifications (and possibly the
GET replies if client opts for it), this format still takes 12-16 bytes
per bit.

So the problem I'm trying to address is that there are two types of
clients with very different mode of work and different preferences.

Looking at the bitset.c, I would rather say that most of the complexity
and ugliness comes from dealing with both unsigned long based bitmaps
and u32 based ones. Originally, there were functions working with
unsigned long based bitmaps and the variants with "32" suffix were
wrappers around them which converted u32 bitmaps to unsigned long ones
and back. This became a problem when kernel started issuing warnings
about variable length arrays as getting rid of them meant two kmalloc()
and two kfree() for each u32 bitmap operation, even if most of the
bitmaps are in rather short in practice.

Maybe the wrapper could do something like

int ethnl_put_bitset32(const u32 *value, const u32 *mask,
	       unsigned int size,  ...)
{
unsigned long fixed_value[2], fixed_mask[2];
unsigned long *tmp_value = fixed_value;
unsigned long *tmp_mask = fixed_mask;

if (size > sizeof(fixed_value) * BITS_PER_BYTE) {
	tmp_value = bitmap_alloc(size);
	if (!tmp_value)
		return -ENOMEM;
	tmp_mask = bitmap_alloc(size);
	if (!tmp_mask) {
		kfree(tmp_value);
		return -ENOMEM;
	}
}

bitmap_from_arr32(tmp_value, value, size);
bitmap_from_arr32(tmp_mask, mask, size);
ret = ethnl_put_bitset(tmp_value, tmp_mask, size, ...);
}

This way we would make bitset.c code cleaner while avoiding allocating
short bitmaps (which is the most common case). 

I'm primarily concerned about the uapi. Plus if the uapi approach is united
for both index and string, we can omit this whole bitset abomination...

I'm afraid I don't understand this comment. Whatever the representation
of bitmaps (both simple bitmaps and value/mask pairs) is going to be, we
will need a function for parsing them (currently ethnl_update_bitset())
and a function for filling them into the message (currently
ethnl_put_bitset()). Unless you are suggesting to write a copy of
essentially the same parser and composer for each of the bitsets (there
is 15 of them at the already and 4 NLA_BITFIELD32 attributes which I'm
seriously considering to replace with arbitrary length bitsets as well
to make the UAPI as future proof as possible).

After all, what you suggested above is exactly the same structure as my
bitset in verbose form, except you omit size (which is a problem, as
discussed in other part of the thread) and put the contents of BITS
container directly under the main container.

Michal