lakshmi.n_msystechnologies.com added a comment.
Thanks @smh, @melifaro and @hrs for the review comments.
REPOSITORY
rS FreeBSD src repository
REVISION DETAIL
https://reviews.freebsd.org/D1986
EMAIL PREFERENCES
https://reviews.freebsd.org/settings/panel/emailpreferences/
To: rpokala,
hrs added inline comments.
INLINE COMMENTS
sys/net/if_lagg.c:753 Please separate a llq loop from a handler for per-port
configuration. A llq traversal should be required only once in lagg_port_ops()
if the handlers process a single lagg_llq entry.
sys/net/if_lagg.c:837 Is this (llq ==
melifaro added inline comments.
INLINE COMMENTS
sys/net/if_lagg.c:728 What if we have multiple events queued on tasq? e.g mtu
AND mac change
sys/net/if_lagg.c:763 Not that easy, unfortunately.
At this moment original ioctl returned 0, so other things/events were fired:
rtsock
smh added a subscriber: smh.
smh added a comment.
Some general style nits and a question re-loss of the mtu set error.
INLINE COMMENTS
sys/net/if_lagg.c:706 style(9) bool use of pointer type.
sys/net/if_lagg.c:731 style(9) four space additional indent only should be
used, more below.
lakshmi.n_msystechnologies.com added a comment.
Thanks @hrs and @melifaro for the suggestions.
I am working on the code changes to handle the MTU asynchronously. I will
update the tested patch for review by this week.
REPOSITORY
rS FreeBSD src repository
REVISION DETAIL
melifaro added a subscriber: melifaro.
melifaro added a comment.
After digging into lagg internals on updating lladdrs on lagg ports, I'd also
vote for extenging llq to deal with MTU changes for underlying interfaces
REPOSITORY
rS FreeBSD src repository
REVISION DETAIL
lakshmi.n_msystechnologies.com added a comment.
@sbruno, A gentle remainder on em driver's LOR issue. Can you please share your
findings.
REPOSITORY
rS FreeBSD src repository
REVISION DETAIL
https://reviews.freebsd.org/D1986
EMAIL PREFERENCES
hrs added a subscriber: hrs.
hrs added a comment.
It is true that this LOR is driver-specific but calling SIOCSIFMTU after
acquiring a lock in lagg ioctl is not always safe. Change of lladdr suffers
from the same situation and it was solved by using an asynchronous task queue
to update
sbruno added a subscriber: sbruno.
sbruno added a comment.
In https://reviews.freebsd.org/D1986#77466, @lakshmi.n_msystechnologies.com
wrote:
> We (Panasas) tried reproducing the problem by applying the patch and enabled
> WITNESS in freebsd stable (10.1) kernel.
> While testing with this
lakshmi.n_msystechnologies.com added a subscriber:
lakshmi.n_msystechnologies.com.
lakshmi.n_msystechnologies.com added a comment.
We (Panasas) tried reproducing the problem by applying the patch and enabled
WITNESS in freebsd stable (10.1) kernel.
While testing with this patched kernel, LOR
rpokala-panasas.com added a comment.
One of my colleagues dug into the first LOR - the one that happens when adding
em1 to lagg0, which happens both with and without the patch. It looks like this
is a known issue:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=194109
REVISION DETAIL
rpokala-panasas.com added a comment.
4079 addresses @rstone's comments. However, the LOR remains:
```
Mar 2 17:00:43 fbsd-X root: ifconfig lagg0 mtu 9000
Mar 2 17:00:43 fbsd-X kernel: lock order reversal:
Mar 2 17:00:43 fbsd-X kernel: 1st 0xf80004960c08 if_lagg rmlock (if_lagg
rmlock) @
rpokala-panasas.com added a comment.
! In D1986#4, @ae wrote:
Just a thought. Imagine two interfaces, one has maximum MTU 2200, another
1500. lagg0 has MTU 1400.
Two threads invokes changing MTU in the same time. One wants to change it to
2000, another - to 1500.
It is possible, that when
rstone added inline comments.
INLINE COMMENTS
sys/net/if_lagg.c:1772 style(9) says to not include unnecessary braces (which
I personally disagree with, but what can you do?)
sys/net/if_lagg.c:1773 style(9): put brackets around the return value:
return (0);
sys/net/if_lagg.c:1811 I
ae added a comment.
! In D1986#7, @rstone wrote:
RLOCK only gets a read lock. You want WLOCK to get a write lock to ensure
serialization.
Also we can use another lock in the lagg_ioctl, that will prevent simultaneous
MTU changing.
REVISION DETAIL
https://reviews.freebsd.org/D1986
To:
15 matches
Mail list logo