change ifmedia word from int to uint64_t

[Masanobu SAITOH]

Hi.

I'm now working to extend ifmedia word to be able to support
more than 10Gbps media.

Patch:

http://www.netbsd.org/~msaitoh/ifmedia64-20190418-0.dif

I changed ifmedia's word from int to uint64_t. It's almost the
same as OpenBSD who did 5 years ago.

What do you think about the above diff?
IMHO, it would be better to change ifmword_t(with uint64_t)
than uint64_t. For example

(from ifconfig/media.c's diff)

@@ -374,7 +374,8 @@ media_status(prop_dictionary_t env, prop
/* Interface link status is queried through SIOCGIFMEDIA.
* Not all interfaces have actual media. */
if (ifmr.ifm_count != 0) {
- media_list = (int *)malloc(ifmr.ifm_count * sizeof(int));
+ media_list = (uint64_t *)malloc(ifmr.ifm_count
+ * sizeof(*media_list));
if (media_list == NULL)
err(EXIT_FAILURE, "malloc");
ifmr.ifm_ulist = media_list;

If we use ifmword_t, it makes easy to change the size in future.


This patch has some TODOs:

- COMPAT_NETBSD32? (currently i386 old binary doesn't work on
amd64)
- I would change the hook point. Current location is not good
because we have to add "case: OSIOC[GS]IFMEDIA" to all driver
which calls ifmedia_ioctl(). (see if_wm.c's patch)


Thanks.

[Christos Zoulas]

Post by Masanobu SAITOH
Hi.
I'm now working to extend ifmedia word to be able to support
more than 10Gbps media.
http://www.netbsd.org/~msaitoh/ifmedia64-20190418-0.dif
I changed ifmedia's word from int to uint64_t. It's almost the
same as OpenBSD who did 5 years ago.
What do you think about the above diff?
IMHO, it would be better to change ifmword_t(with uint64_t)
than uint64_t. For example
(from ifconfig/media.c's diff)

@@ -374,7 +374,8 @@ media_status(prop_dictionary_t env, prop
/* Interface link status is queried through SIOCGIFMEDIA.
* Not all interfaces have actual media. */
if (ifmr.ifm_count != 0) {
- media_list = (int *)malloc(ifmr.ifm_count * sizeof(int));
+ media_list = (uint64_t *)malloc(ifmr.ifm_count
+ * sizeof(*media_list));
if (media_list == NULL)
err(EXIT_FAILURE, "malloc");
ifmr.ifm_ulist = media_list;

If we use ifmword_t, it makes easy to change the size in future.
- COMPAT_NETBSD32? (currently i386 old binary doesn't work on
amd64)
- I would change the hook point. Current location is not good
because we have to add "case: OSIOC[GS]IFMEDIA" to all driver
which calls ifmedia_ioctl(). (see if_wm.c's patch)

- Casts to malloc are harmful.
- malloc with multiplication should be converted to calloc which
checks for overflow.

christos


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Masanobu SAITOH]

Post by Christos Zoulas

Post by Masanobu SAITOH
Hi.
I'm now working to extend ifmedia word to be able to support
more than 10Gbps media.
http://www.netbsd.org/~msaitoh/ifmedia64-20190418-0.dif
I changed ifmedia's word from int to uint64_t. It's almost the
same as OpenBSD who did 5 years ago.
What do you think about the above diff?
IMHO, it would be better to change ifmword_t(with uint64_t)
than uint64_t. For example
(from ifconfig/media.c's diff)

@@ -374,7 +374,8 @@ media_status(prop_dictionary_t env, prop
/* Interface link status is queried through SIOCGIFMEDIA.
* Not all interfaces have actual media. */
if (ifmr.ifm_count != 0) {
- media_list = (int *)malloc(ifmr.ifm_count * sizeof(int));
+ media_list = (uint64_t *)malloc(ifmr.ifm_count
+ * sizeof(*media_list));
if (media_list == NULL)
err(EXIT_FAILURE, "malloc");
ifmr.ifm_ulist = media_list;

If we use ifmword_t, it makes easy to change the size in future.
- COMPAT_NETBSD32? (currently i386 old binary doesn't work on
amd64)
- I would change the hook point. Current location is not good
because we have to add "case: OSIOC[GS]IFMEDIA" to all driver
which calls ifmedia_ioctl(). (see if_wm.c's patch)

- Casts to malloc are harmful.
- malloc with multiplication should be converted to calloc which
checks for overflow.

Done in -current.

Thanks!

Post by Christos Zoulas
christos

[Jason Thorpe]

Post by Masanobu SAITOH
Hi.
I'm now working to extend ifmedia word to be able to support
more than 10Gbps media.
http://www.netbsd.org/~msaitoh/ifmedia64-20190418-0.dif

It might be nice to use __BIT(), __SHIFT*() etc. macros (if they can do ULL constants)... I think it would make all of the constants a lot easier to read.

I suppose we could add an ifmword_t ... but if we have to enlarge it again, it's going to become a struct (or something else entirely), right? I guess it makes the "grep" phase of the next overhaul a little easier :-)

Post by Masanobu SAITOH
I changed ifmedia's word from int to uint64_t. It's almost the
same as OpenBSD who did 5 years ago.
What do you think about the above diff?
IMHO, it would be better to change ifmword_t(with uint64_t)
than uint64_t. For example
(from ifconfig/media.c's diff)

@@ -374,7 +374,8 @@ media_status(prop_dictionary_t env, prop
/* Interface link status is queried through SIOCGIFMEDIA.
* Not all interfaces have actual media. */
if (ifmr.ifm_count != 0) {
- media_list = (int *)malloc(ifmr.ifm_count * sizeof(int));
+ media_list = (uint64_t *)malloc(ifmr.ifm_count
+ * sizeof(*media_list));
if (media_list == NULL)
err(EXIT_FAILURE, "malloc");
ifmr.ifm_ulist = media_list;

If we use ifmword_t, it makes easy to change the size in future.
- COMPAT_NETBSD32? (currently i386 old binary doesn't work on
amd64)
- I would change the hook point. Current location is not good
because we have to add "case: OSIOC[GS]IFMEDIA" to all driver
which calls ifmedia_ioctl(). (see if_wm.c's patch)
Thanks.
--
-----------------------------------------------

-- thorpej


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Jason Thorpe]

Post by Jason Thorpe

Post by Masanobu SAITOH
Hi.
I'm now working to extend ifmedia word to be able to support
more than 10Gbps media.
http://www.netbsd.org/~msaitoh/ifmedia64-20190418-0.dif

It might be nice to use __BIT(), __SHIFT*() etc. macros (if they can do ULL constants)... I think it would make all of the constants a lot easier to read.
I suppose we could add an ifmword_t ... but if we have to enlarge it again, it's going to become a struct (or something else entirely), right? I guess it makes the "grep" phase of the next overhaul a little easier :-)

Also:

Index: sys/sys/sockio.h
===================================================================
RCS file: /cvsroot/src/sys/sys/sockio.h,v
retrieving revision 1.36
diff -u -p -r1.36 sockio.h
--- sys/sys/sockio.h 21 Dec 2018 08:58:08 -0000 1.36
+++ sys/sys/sockio.h 18 Apr 2019 09:12:46 -0000
@@ -90,8 +90,9 @@
#define SIOCGETVIFCNT _IOWR('u', 51, struct sioc_vif_req)/* vif pkt cnt */
#define SIOCGETSGCNT _IOWR('u', 52, struct sioc_sg_req) /* sg pkt cnt */

-#define SIOCSIFMEDIA _IOWR('i', 53, struct ifreq) /* set net media */
-#define SIOCGIFMEDIA _IOWR('i', 54, struct ifmediareq) /* get net media */
+/* 53 and 54 used to be SIOC[SG]IFMEDIA with a 32 bit media word */
+#define SIOCSIFMEDIA _IOWR('i', 55, struct ifreq) /* set net media */
+#define SIOCGIFMEDIA _IOWR('i', 56, struct ifmediareq) /* get net media */

#define SIOCSIFGENERIC _IOW('i', 57, struct ifreq) /* generic IF set op */
#define SIOCGIFGENERIC _IOWR('i', 58, struct ifreq) /* generic IF get op */

You don't have to allocate new numbers; the size of the argument structure changes, and therefore the value of SIOCSIFMEDIA / SIOCGIFMEDIA changes.

Post by Jason Thorpe

Post by Masanobu SAITOH
I changed ifmedia's word from int to uint64_t. It's almost the
same as OpenBSD who did 5 years ago.
What do you think about the above diff?
IMHO, it would be better to change ifmword_t(with uint64_t)
than uint64_t. For example
(from ifconfig/media.c's diff)

@@ -374,7 +374,8 @@ media_status(prop_dictionary_t env, prop
/* Interface link status is queried through SIOCGIFMEDIA.
* Not all interfaces have actual media. */
if (ifmr.ifm_count != 0) {
- media_list = (int *)malloc(ifmr.ifm_count * sizeof(int));
+ media_list = (uint64_t *)malloc(ifmr.ifm_count
+ * sizeof(*media_list));
if (media_list == NULL)
err(EXIT_FAILURE, "malloc");
ifmr.ifm_ulist = media_list;

If we use ifmword_t, it makes easy to change the size in future.
- COMPAT_NETBSD32? (currently i386 old binary doesn't work on
amd64)
- I would change the hook point. Current location is not good
because we have to add "case: OSIOC[GS]IFMEDIA" to all driver
which calls ifmedia_ioctl(). (see if_wm.c's patch)
Thanks.
--
-----------------------------------------------

-- thorpej

-- thorpej


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Masanobu SAITOH]

Hi, Jason.

Post by Jason Thorpe

Post by Masanobu SAITOH
Hi.
I'm now working to extend ifmedia word to be able to support
more than 10Gbps media.
http://www.netbsd.org/~msaitoh/ifmedia64-20190418-0.dif

It might be nice to use __BIT(), __SHIFT*() etc. macros (if they can do ULL constants)... I think it would make all of the constants a lot easier to read.

I usually use __BIT() macros for 32bit values, but I sometimes
hesitate to use it for 64bit (or 16bit) values because man bit(3)'s
BUGS section says...

http://cvsweb.netbsd.org/bsdweb.cgi/src/share/man/man3/bits.3.diff?r1=1.16&r2=1.17&f=h

Oops. I had not known the BUGS sections was removed until now!
BTW, is ti safe for 16bit values? mii(4)'s register definitions
don't use __BIT(). If it's usable I might modify to use it.

Post by Jason Thorpe
I suppose we could add an ifmword_t ... but if we have to enlarge it again, it's going to become a struct (or something else entirely), right?

Right. Though I didn't write about my idea in the previous mail,
I'd like to split the ifmedia word into two part at least. One
is to just select media type and another is for control options.
It's a future work.

Post by Jason Thorpe
I guess it makes the "grep" phase of the next overhaul a little easier :-)

If we change the declaration twice, 3rd party program might confuse and
be complex. So, IMHO, It would be good to just change int to uint64_t
this time. What do you think about this idea?

Post by Jason Thorpe

Post by Masanobu SAITOH
I changed ifmedia's word from int to uint64_t. It's almost the
same as OpenBSD who did 5 years ago.
What do you think about the above diff?
IMHO, it would be better to change ifmword_t(with uint64_t)
than uint64_t. For example
(from ifconfig/media.c's diff)

@@ -374,7 +374,8 @@ media_status(prop_dictionary_t env, prop
/* Interface link status is queried through SIOCGIFMEDIA.
* Not all interfaces have actual media. */
if (ifmr.ifm_count != 0) {
- media_list = (int *)malloc(ifmr.ifm_count * sizeof(int));
+ media_list = (uint64_t *)malloc(ifmr.ifm_count
+ * sizeof(*media_list));
if (media_list == NULL)
err(EXIT_FAILURE, "malloc");
ifmr.ifm_ulist = media_list;

If we use ifmword_t, it makes easy to change the size in future.
- COMPAT_NETBSD32? (currently i386 old binary doesn't work on
amd64)
- I would change the hook point. Current location is not good
because we have to add "case: OSIOC[GS]IFMEDIA" to all driver
which calls ifmedia_ioctl(). (see if_wm.c's patch)
Thanks.
--
-----------------------------------------------

-- thorpej

[Masanobu SAITOH]

Post by Jason Thorpe

Post by Jason Thorpe

Post by Masanobu SAITOH
Hi.
I'm now working to extend ifmedia word to be able to support
more than 10Gbps media.
http://www.netbsd.org/~msaitoh/ifmedia64-20190418-0.dif

It might be nice to use __BIT(), __SHIFT*() etc. macros (if they can do ULL constants)... I think it would make all of the constants a lot easier to read.
I suppose we could add an ifmword_t ... but if we have to enlarge it again, it's going to become a struct (or something else entirely), right? I guess it makes the "grep" phase of the next overhaul a little easier :-)

Index: sys/sys/sockio.h
===================================================================
RCS file: /cvsroot/src/sys/sys/sockio.h,v
retrieving revision 1.36
diff -u -p -r1.36 sockio.h
--- sys/sys/sockio.h 21 Dec 2018 08:58:08 -0000 1.36
+++ sys/sys/sockio.h 18 Apr 2019 09:12:46 -0000
@@ -90,8 +90,9 @@
#define SIOCGETVIFCNT _IOWR('u', 51, struct sioc_vif_req)/* vif pkt cnt */
#define SIOCGETSGCNT _IOWR('u', 52, struct sioc_sg_req) /* sg pkt cnt */
-#define SIOCSIFMEDIA _IOWR('i', 53, struct ifreq) /* set net media */
-#define SIOCGIFMEDIA _IOWR('i', 54, struct ifmediareq) /* get net media */
+/* 53 and 54 used to be SIOC[SG]IFMEDIA with a 32 bit media word */
+#define SIOCSIFMEDIA _IOWR('i', 55, struct ifreq) /* set net media */
+#define SIOCGIFMEDIA _IOWR('i', 56, struct ifmediareq) /* get net media */
#define SIOCSIFGENERIC _IOW('i', 57, struct ifreq) /* generic IF set op */
#define SIOCGIFGENERIC _IOWR('i', 58, struct ifreq) /* generic IF get op */
You don't have to allocate new numbers; the size of the argument structure changes, and therefore the value of SIOCSIFMEDIA / SIOCGIFMEDIA changes.

Good catch! SIOCGIFMEDIA will have different size, so I'll keep the number.
For SIOCSIFMEDIA, the ifreq's member is union-ed and the max size is greater
than uint64_t. I think this new ioctl's number must be changed.

Post by Jason Thorpe

Post by Jason Thorpe

Post by Masanobu SAITOH
I changed ifmedia's word from int to uint64_t. It's almost the
same as OpenBSD who did 5 years ago.
What do you think about the above diff?
IMHO, it would be better to change ifmword_t(with uint64_t)
than uint64_t. For example
(from ifconfig/media.c's diff)

@@ -374,7 +374,8 @@ media_status(prop_dictionary_t env, prop
/* Interface link status is queried through SIOCGIFMEDIA.
* Not all interfaces have actual media. */
if (ifmr.ifm_count != 0) {
- media_list = (int *)malloc(ifmr.ifm_count * sizeof(int));
+ media_list = (uint64_t *)malloc(ifmr.ifm_count
+ * sizeof(*media_list));
if (media_list == NULL)
err(EXIT_FAILURE, "malloc");
ifmr.ifm_ulist = media_list;

If we use ifmword_t, it makes easy to change the size in future.
- COMPAT_NETBSD32? (currently i386 old binary doesn't work on
amd64)
- I would change the hook point. Current location is not good
because we have to add "case: OSIOC[GS]IFMEDIA" to all driver
which calls ifmedia_ioctl(). (see if_wm.c's patch)
Thanks.
--
-----------------------------------------------

-- thorpej

-- thorpej

[Masanobu SAITOH]

(I changed the Subject)

Hi, all.

I'm working to overhaul ifmedia(4) and the lower layer. Changing the
ifmedia word from int to uint64_t was one of them. If I commit it to
current and then netbsd-9 is branched, Other changes will be added
to -current in future. It will make the ABI change twice. It makes
the compat code in if.c and ifmedia.c very complex and also makes
some 3rd party programs complex.

So, I gave up to change the ifmedia word from int to uint64_t. Instead
of it, I'd like to change the ifmedia word not changing the size but
changing the usage usage only on IFM_ETHER.

Diff:
http://www.netbsd.org/~msaitoh/ifmedia-20190513-0.dif

The strategy is almost the same as FreeBSD. Many bits of IFM_OMASK
for Ethernet have not used, some of them are used. The differences against
FreeBSD are:

- We use NetBSD style compat code. Don't use SIOCGIFXMEDIA.
The old ioctls are prefixed with 'O'.
- New IFM_ETH_XTYPE's bit location in FreeBSD is little strange.
We use from bit 13 to 15.
- FreeBSD changed the meaning of IFM_TYPE_MATCH(). I think we should
not do it. We keep it not changing and added new IFM_TYPE_SUBTYPE_MATCH()
macro for matching both TYPE and SUBTYPE.
- Added up to 400GBASE-SR16.

The above diff includes a test patch in if_tap.c. I tested the
following environment on the new kernel:

- New amd64 dynamic link ifconfig(8)
- netbsd-8 amd64 dynamic link ifconfig(8)
- netbsd-7 i386 static link ifconfig(8)
- netbsd-4 amd64 dynamic link ifconfig(8) (for COMPAT_43 testing)


I think this change isn't harmful, so I'll commit the change in
this weekend if no one objects. Feedback are welcomed.

Thanks in advance.

[Edgar Fuß]

Post by Masanobu SAITOH
The old ioctls are prefixed with 'O'.

I remember a long post by kre@ why this tradition shouldn't be continued.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Masanobu SAITOH]

Post by Masanobu SAITOH
The old ioctls are prefixed with 'O'.

?

http://gnats.netbsd.org/54150

This one?

Isn't that deny any type of OXXXX change, is that?

[Edgar Fuß]

Post by Masanobu SAITOH
This one?

No, http://mail-index.netbsd.org/tech-net/2019/05/01/msg007341.html.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Robert Elz]

Date: Mon, 13 May 2019 18:22:28 +0900
From: Masanobu SAITOH <***@execsw.org>
Message-ID: <6bc1e74a-efda-6bef-d478-***@execsw.org>

| Isn't that deny any type of OXXXX change, is that?

Yes, but only the name ... the name you use to rename the old ioctl's
(and functions, structs, enums, ... anything that needs it) for use
by the in kernel compat code, and nothing else, is really irrelevant,
you could rename SIOCFOOBAR into "dead_frog" and everything would work
just as well.

But not everything is as easily able to be maintained into the future.

The 'shove in an O' convention came from CSRG in the 80's - when no-one
had any idea how long this ancient stuff would be being maintained, nor
how many times some parts would be revised, and revised again (now have OO)
abnd again (OOO). That's a nightmare.

If instead of s/SIOCFOOBAR/SIOCOFOOBAR/ (or similar) you do
s/SIOCFOOBAR/SIOCFOOBAR_80/ (or whichever most recent released
version uses the ioctl (or whatever) you're changing) then that
name never needs to alter again - and it is blindingly obvious when
reading the code just which system the compat code is keeping track
of, so when the older NetBSD 1.4 code calls the NetBSD 8 code to
do something (and then the 8 code translates that to current) it
is clear to anyone looking exactly what is happening - rather than
having to futz round with counting O's and tring to work out for
each identifier (some they don't all get updated in sync) how many
O's represents which version of the code.

kre



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Masanobu SAITOH]

Hi.

Post by Robert Elz
Date: Mon, 13 May 2019 18:22:28 +0900
| Isn't that deny any type of OXXXX change, is that?
Yes, but only the name ... the name you use to rename the old ioctl's
(and functions, structs, enums, ... anything that needs it) for use
by the in kernel compat code, and nothing else, is really irrelevant,
you could rename SIOCFOOBAR into "dead_frog" and everything would work
just as well.
But not everything is as easily able to be maintained into the future.
The 'shove in an O' convention came from CSRG in the 80's - when no-one
had any idea how long this ancient stuff would be being maintained, nor
how many times some parts would be revised, and revised again (now have OO)
abnd again (OOO). That's a nightmare.
If instead of s/SIOCFOOBAR/SIOCOFOOBAR/ (or similar) you do
s/SIOCFOOBAR/SIOCFOOBAR_80/ (or whichever most recent released
version uses the ioctl (or whatever) you're changing) then that
name never needs to alter again - and it is blindingly obvious when
reading the code just which system the compat code is keeping track
of, so when the older NetBSD 1.4 code calls the NetBSD 8 code to
do something (and then the 8 code translates that to current) it
is clear to anyone looking exactly what is happening - rather than
having to futz round with counting O's and tring to work out for
each identifier (some they don't all get updated in sync) how many
O's represents which version of the code.
kre

Thank you for the clarify. I fully agreed with it. It makes:

- not required to gaze to know the number of 'O's.
- not required grep old macros and replace all of them and
only required to add newly old name.
- not required to search the code to know which release is the
last release of the old ABI and just see the _XX number.

New diff:
http://www.netbsd.org/~msaitoh/ifmedia-20190514-0.dif

I renamed the following macros from the last patch:
OOSIOCSIFMEDIA -> SIOCSIFMEDIA_43
OSIOCSIFMEDIA -> SIOCSIFMEDIA_80
OSIOCGIFMEDIA -> SIOCGIFMEDIA_80
OSIOCGIFMEDIA32 -> SIOCGIFMEDIA32_80

[Masanobu SAITOH]

Post by Masanobu SAITOH
http://www.netbsd.org/~msaitoh/ifmedia-20190514-0.dif

I'll commit the above new diff tomorrow if anyone object.

[Masanobu SAITOH]

Post by Masanobu SAITOH

Post by Masanobu SAITOH
http://www.netbsd.org/~msaitoh/ifmedia-20190514-0.dif

I'll commit the above new diff tomorrow if anyone object.

Done.

Thanks all.

[Jason Thorpe]

Post by Masanobu SAITOH

Post by Masanobu SAITOH
http://www.netbsd.org/~msaitoh/ifmedia-20190514-0.dif

I'll commit the above new diff tomorrow if anyone object.

Done.

Thanks for doing this work!

-- thorpej


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de